-
Notifications
You must be signed in to change notification settings - Fork 3
feat: move column toggle into a slot AND feat: replace column toggle icon #180
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,7 +2,7 @@ | |
| * #%L | ||
| * Grid Helpers Add-on | ||
| * %% | ||
| * Copyright (C) 2022 - 2025 Flowing Code | ||
| * Copyright (C) 2022 - 2026 Flowing Code | ||
| * %% | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); | ||
| * you may not use this file except in compliance with the License. | ||
|
|
@@ -50,7 +50,6 @@ | |
|
|
||
| @SuppressWarnings("serial") | ||
| @JsModule("./fcGridHelper/connector.js") | ||
| @CssImport(value = "./fcGridHelper/vaadin-menu-bar.css", themeFor = "vaadin-menu-bar") | ||
| @CssImport(value = GridHelper.GRID_STYLES, themeFor = "vaadin-grid") | ||
| @CssImport( | ||
| value = "./fcGridHelper/vaadin-context-menu-item.css", | ||
|
|
@@ -67,6 +66,7 @@ | |
| @CssImport( | ||
| value = "./fcGridHelper/vaadin-checkbox.css", | ||
| themeFor = "vaadin-checkbox") | ||
| @CssImport(value = "./fcGridHelper/styles.css") | ||
| public final class GridHelper<T> implements Serializable { | ||
|
|
||
| private static final Logger logger = LoggerFactory.getLogger(GridHelper.class); | ||
|
|
@@ -362,8 +362,9 @@ | |
| return getHelper(column.getGrid()).columnToggleHelper.getHidingToggleCaption(column); | ||
| } | ||
|
|
||
| @Deprecated(forRemoval = true, since = "2.2.0") | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. GridHelper.isMenuToggleColumn has no @deprecated javadoc explaining that it now always returns false. Worth adding, together with a note that the parameter is ignored: the method used to dereference column (via getHelper(column)) and therefore threw on null, and it now silently returns false instead. |
||
| public static boolean isMenuToggleColumn(Column<?> column) { | ||
|
Check warning on line 366 in src/main/java/com/flowingcode/vaadin/addons/gridhelpers/GridHelper.java
|
||
| return column == getHelper(column).columnToggleHelper.getMenuToggleColumn(); | ||
| return false; | ||
|
coderabbitai[bot] marked this conversation as resolved.
|
||
| } | ||
|
|
||
| // Empty Label | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -29,7 +29,12 @@ import { Grid } from '@vaadin/grid/src/vaadin-grid.js'; | |
| (function () { | ||
| window.Vaadin.Flow.fcGridHelperConnector = { | ||
| initLazy: grid => { | ||
|
|
||
| if (!grid.shadowRoot.querySelector('slot[name="fc-column-toggle"]')) { | ||
| const slot = document.createElement('slot'); | ||
| slot.setAttribute('name','fc-column-toggle') | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Semicolon is missing at the end of line. |
||
| grid.shadowRoot.appendChild(slot); | ||
| } | ||
|
|
||
| //https://cookbook.vaadin.com/grid-arrow-selection | ||
| grid.addEventListener('keyup', function(e) { | ||
| if (e.keyCode == 32) return; | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -47,16 +47,24 @@ public void setup() throws Exception { | |
|
|
||
| @Test | ||
| public void testColumnToggleVisible() { | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. testColumnToggleVisible dropped its column-count assertions, so nothing verifies the actual fix: that showing the toggle no longer adds a column. Asserting that getVisibleColumns() has the same size before and after setColumnToggleVisible(true) would guard the regression directly. |
||
| int nColumns = grid.getVisibleColumns().size(); | ||
| assertNull("ColumnToggle should be absent", grid.getColumnToggleButton()); | ||
|
|
||
| $server.setColumnToggleVisible(true); | ||
| assertNotNull("ColumnToggle should be present", grid.getColumnToggleButton()); | ||
| assertThat(grid.getVisibleColumns(), hasSize(nColumns + 1)); | ||
|
|
||
| $server.setColumnToggleVisible(false); | ||
| assertNull("ColumnToggle should be absent", grid.getColumnToggleButton()); | ||
| assertThat(grid.getVisibleColumns(), hasSize(nColumns)); | ||
| } | ||
|
|
||
| @Test | ||
| public void testColumnToggleSlot() { | ||
| $server.setColumnToggleVisible(true); | ||
| assertNotNull("ColumnToggle should be present", grid.getColumnToggle()); | ||
| assertEquals("fc-column-toggle", grid.getColumnToggleSlotName()); | ||
|
|
||
| $server.setColumnToggleVisible(false); | ||
| assertNull("ColumnToggle should be absent", grid.getColumnToggle()); | ||
| assertNull(grid.getColumnToggleSlotName()); | ||
| } | ||
|
|
||
| @Test | ||
|
|
@@ -66,9 +74,7 @@ public void testColumnToggleClick() { | |
| $server.setColumnToggleVisible(true); | ||
| grid.getColumnToggleButton().click(); | ||
|
|
||
| // the toggle is rendered in its own column | ||
| assertThat(grid.getColumnToggleElements(), hasSize(nColumns)); | ||
| assertThat(grid.getVisibleColumns(), hasSize(++nColumns)); | ||
|
|
||
| grid.getColumnToggleElements().get(0).setChecked(false); | ||
| assertThat(grid.getVisibleColumns(), hasSize(nColumns - 1)); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changing away from ⋮ matches what the issue asked for a bare ⋮ reads as "more actions" and doesn't hint that columns can be hidden. But I don't think CARET_DOWN is the right replacement:
Also and more important: there is no API to override the icon, so every existing user gets the new appearance on upgrade with no way back. Not an API break, and the minor bump to 2.2.0 is right, but it should be called out in the release notes. And the possibility to override it should be considered as a future enhancement.