Add select-all checkbox to row filter#21
Conversation
There was a problem hiding this comment.
Thanks, this completes #19 and the value-list master feels right. Mergeable, no wrong filter results in the common path. After a closer look the cleanest path is to keep the scoped master and relabel it to 'Select all matches' when a search is active, which is the Excel model and resolves the scope and blank questions in one small change. Details inline, plus a scope note on the CSS block.
| }); | ||
| deselAll.addEventListener('click', () => { | ||
| this.checkedValues.clear(); | ||
| masterCb.addEventListener('change', () => { |
There was a problem hiding this comment.
Follow-up on the blank value: with the 'Select all matches' label above, leaving (Blank) unselected while a non-matching search is active is actually correct, because (Blank) is not a match. No special-case is needed here. A user who wants blanks back just clears the search and ticks (Blank). I had first suggested force-adding blank in this handler, but that only fits the old global model and would be inconsistent under scoping (it touches a non-match). So the label change is the real fix. This handler can stay as is.
There was a problem hiding this comment.
Fixed, the master now re-adds blank via _showBlankInList() when checking, mirroring the old Select All.
| listDiv.className = 'csv-filter-values-list'; | ||
| valSec.appendChild(listDiv); | ||
|
|
||
| const syncMaster = () => { |
There was a problem hiding this comment.
Recommendation: keep the scoped behavior and make the scope explicit here. syncMaster computes checked and total over _displayedValues while isFilterActive() works over allValues, so with a search active the master can read a full 'N / N' while the column is still filtered. The clean fix is the label: when _searchQuery is non-empty, set the master label to 'Select all matches' and back to 'Select all' when empty. Then 'N / N matches checked' is honest, the column funnel icon stays as the real 'this column is filtered' indicator and nothing about the value logic changes. This is exactly what Excel does with '(Select All Search Results)'.
There was a problem hiding this comment.
Confirmed, this logic is intentional. The master record reflects the scoped/filtered values currently on display, similar to Excel, rather than the column's filter status.
| color: var(--vscode-charts-orange, #d18616) !important; | ||
| font-weight: 700 !important; | ||
| } | ||
| /* Override AG Grid's default Alpine popup styles to match the extension's popovers. */ |
There was a problem hiding this comment.
Two small things on this trailing block. It styles the AG Grid filter and menu popup, which is unrelated to the select-all feature and overlaps with the Escape work in #22, so it reads as scope creep here. And the file lost its trailing newline after the closing brace (the diff shows 'No newline at end of file'). Could you split the AG popup styling out or fold it into #22 and restore the final newline.
There was a problem hiding this comment.
Moved the AG popup styling out of this PR into #22.
|
Thanks for the quick turnaround. The trailing newline fixes, the dedup and moving the AG popup styling into #22 all look good. One thing on the (Blank) handling, and this one is on me because my earlier comment was easy to misread. I think we actually want the same thing here, the scoped Excel model you described in your reply where the master reflects the values currently on display. There is just one line left that pulls the other way. In the master change handler: if (check && this._showBlankInList()) this.checkedValues.add('__blank__');
When (Blank) is visible it is already in The second half of my original comment was the real fix and I do not think it landed yet: when a search is active, switch the master label to "Select all matches" and back to "Select all" when the search is empty. That makes the scoped count honest, N / N then reads as "all matches selected" while the column funnel icon stays the real "this column is filtered" signal. Same idea applies to the column chooser in #20. No rush on the final newline, I will sort the merge order so #21 and #22 do not clash on webview.css. Thanks again, this is close. |
|
I've removed the redundant _showBlankInList() line and implemented the dynamic label switch to (Select all matches) when a search is active. |
2 participants
Completes #19 by replacing "Select All / Deselect All" with a tri-state master checkbox. It targets only visible results matching active filters and search. Updates are fast since they only run on visible items, avoiding rebuilding the list or breaking existing filter logic.