rebenitez1802 commented on code in PR #43460:
URL: https://github.com/apache/superset/pull/43460#discussion_r3870987504
##########
superset-frontend/packages/superset-ui-core/src/components/Select/Select.tsx:
##########
@@ -293,6 +300,19 @@ const Select = forwardRef(
[visibleOptions],
);
+ // The stable, full-set counterpart of enabledOptions: every
bulk-selectable
+ // option across the entire (search-independent) option set. Used when
+ // stableSelectAll is on so the "Select all" action and visibility stay
+ // pinned to the full column while a search narrows visibleOptions. Gated
on
+ // stableSelectAll so consumers that don't use the feature skip the filter.
+ const fullSelectAllOptions = useMemo(
+ () =>
+ stableSelectAll
+ ? fullSelectOptions.filter(isBulkSelectable)
Review Comment:
Good catch — you're right that `fullSelectOptions` keeps the group nodes
(which carry no value), so the filter came back empty and hid both bulk
actions. Fixed by deriving the full selectable set from the flattened group
children (`flattenGroupedOptions`), reused across the count, `Select all`, and
`Clear` so grouped columns behave like flat ones — it's the identity for a flat
list, so the non-grouped/non-stable paths are unchanged. Added a grouped
select-all test. It's latent today (the native Value filter only emits flat
options, and master's grouped select-all was already inert), but worth
hardening in the shared component. Thanks!
##########
superset-frontend/src/filters/components/Select/SelectFilterPlugin.test.tsx:
##########
@@ -882,6 +882,107 @@ describe('SelectFilterPlugin', () => {
userEvent.type(screen.getByRole('combobox'), 'brand-new');
expect(screen.queryByTitle('brand-new')).not.toBeInTheDocument();
});
+
+ // The native Value filter's "Select all" targets the whole column, so its
+ // count must stay pinned to the full option set while the user searches — no
+ // transient scoped value (regression for Shortcut 115492). This integration
Review Comment:
Done — dropped the internal tracker id and just describe the regression the
test guards instead. Thanks!
##########
superset-frontend/packages/superset-ui-core/src/components/Select/Select.tsx:
##########
@@ -308,49 +328,60 @@ const Select = forwardRef(
!isSingleMode &&
allowSelectAll &&
selectOptions.length > 0 &&
- enabledOptions.length > 1,
+ // When stableSelectAll is on, gate visibility on the full eligible set
+ // so the bulk control does not hide/flicker while a search narrows
+ // visibleOptions.
+ (stableSelectAll
+ ? fullSelectAllOptions.length
+ : enabledOptions.length) > 1,
Review Comment:
Agreed — and thanks for the thorough verification. Master's bulk control on
these columns was itself misleading (it counts falsy values that "Select all"
can never actually add), so hiding it removes a deceptive control rather than a
working one. The filter stays fully usable via per-option selection and the
top-level clear, so I'm keeping it as-is.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
