Add select-all checkbox to column chooser#20
Conversation
There was a problem hiding this comment.
Thanks for this. The tri-state master is a clean replacement for the two buttons and it matches #19 well. Mergeable, no correctness bug. After a closer look my recommendation is to keep the scoped behavior and relabel the master to 'Select all matches' when a search is active, the Excel way, which makes the count and checked state honest. Details inline, plus a small cleanup and a trailing-newline nit.
| state.hiddenCols.clear(); | ||
| for (let c = 0; c < n; c++) state.hiddenCols.add(c); | ||
| state.gridApi?.setColumnsVisible(allColIds(), false); | ||
| function setVisibleColsHidden(hidden: boolean): void { |
There was a problem hiding this comment.
Recommendation after a closer look: keep this scoped behavior (it is the Excel model #19 asked for) and just make the scope visible. The only gap is that the label stays 'Select all' while the action is scoped to the search. In syncMaster, when searchQuery is non-empty, set the master label to 'Select all matches' and back to 'Select all' when the search is empty. That makes the N / N count and the checked state honest. The global escape hatch already exists since clearing the search box restores the global master, so no separate Show-all button is needed (that was the clutter #19 wanted gone).
There was a problem hiding this comment.
Brought back the global reveal behavior. Checking the master box with an empty search now clears all hidden items (Show all). When filtered, it stays scoped to the search results.
| state.gridApi?.setColumnsVisible(allColIds(), true); | ||
| buildList(); | ||
| updateButton(); | ||
| function visibleColIndices(): number[] { |
There was a problem hiding this comment.
Small cleanup: visibleColIndices() now owns the search predicate (q && !colLabel(...).toLowerCase().includes(q)), but buildList() still runs a byte-identical copy of it in its own render loop. No bug today, but the master count (from visibleColIndices) and the rendered rows (from buildList) derive 'which columns match' from two copies of the rule. If the match logic changes in one place they will disagree. Could buildList iterate over visibleColIndices() so there is one source of truth.
There was a problem hiding this comment.
buildList() now iterates visibleColIndices(), so the search-match rule lives in one place.
| closeChooser(); | ||
| }, true); | ||
| } | ||
| } No newline at end of file |
There was a problem hiding this comment.
Nit: the file lost its trailing newline here (the diff shows 'No newline at end of file'). Every other file in src/webview/features ends with one, so this shows up as a noisy one-line diff on the next edit. Mind adding it back.
There was a problem hiding this comment.
Added the trailing newline.
… trailing newline
|
Same note as on #21 about the label: the master still reads a fixed "Select all" while the action is scoped to the search. Could you switch it to "Select all matches" when the search box is non-empty and back to "Select all" when it is cleared, so the N / N count stays honest. Everything else here looks good, the single source of truth in buildList and the global show-all on empty search are exactly right. |
2 participants
Replaces "Show all / Hide all" buttons with an Excel-style tri-state master checkbox (column-chooser part of #19). It controls and reflects only the columns matching the current search. Pinned above the scroll list, toggling individual columns refreshes the master state without rebuilding the list.