michael-s-molina commented on code in PR #22084:
URL: https://github.com/apache/superset/pull/22084#discussion_r1019201572
##########
superset-frontend/src/components/Select/types.ts:
##########
@@ -154,6 +154,16 @@ export interface SelectProps extends BaseSelectProps {
* an array of options.
*/
options: SelectOptionsType;
+ /**
+ * It defines whether select all is an option.
+ * When selected all options are added to the selected
+ * values. Only available in sync select currently.
+ */
+ selectAllEnabled: boolean;
Review Comment:
I think "Select All" shouldn't be optional to keep the UI consistent
throughout the application. In this first phase, it would be available for all
sync multi-selects so I would remove this property.
##########
superset-frontend/src/components/Select/utils.tsx:
##########
@@ -25,6 +25,12 @@ import { LabeledValue, RawValue, SelectOptionsType, V } from
'./types';
const { Option } = AntdSelect;
+export const SELECT_ALL_VALUE = 'Select All';
+export const labeledSelectAllOption: AntdLabeledValue = {
Review Comment:
I think you can use `selectAllOption` here for simplicity.
##########
superset-frontend/src/components/Select/Select.test.tsx:
##########
@@ -566,6 +567,13 @@ test('finds an element with a numeric value and does not
duplicate the options',
expect(await querySelectOption('11')).not.toBeInTheDocument();
});
+test('select all appears in options when multi select is true and
selectAllEnabled is true', async () => {
Review Comment:
Can you add the tests from my [original
PR](https://github.com/apache/superset/pull/20143/files#diff-b5bb97e8d6f3ab72182ddecbf206173015f576950a714287f4befc8d1a041e37)?
##########
superset-frontend/src/components/Select/utils.tsx:
##########
@@ -25,6 +25,12 @@ import { LabeledValue, RawValue, SelectOptionsType, V } from
'./types';
const { Option } = AntdSelect;
+export const SELECT_ALL_VALUE = 'Select All';
Review Comment:
For compatibility reasons, I think in this first phase we should send all
values to `onChange` when selecting "Select all". It would be the same as our
current behavior where a user can manually select all items. In the next phase,
we should come up with a smarter way of representing the "Select All" value but
this will require backend changes to interpret this value. My [original
PR](https://github.com/apache/superset/pull/20143) has an item about this:
> Handle the payload size. We shouldn't send all selected items to the
server-side because we don't know how many can exist, which could result in
huge payload size. What we need to do is to come up with a payload that
represents the user selection in an efficient way. One example would be if a
user selects all items and unselects a subset, we could send a value
representing the "Select all" alongside the subset of unselected items.
##########
superset-frontend/src/components/Select/types.ts:
##########
@@ -154,6 +154,16 @@ export interface SelectProps extends BaseSelectProps {
* an array of options.
*/
options: SelectOptionsType;
+ /**
+ * It defines whether select all is an option.
+ * When selected all options are added to the selected
+ * values. Only available in sync select currently.
+ */
+ selectAllEnabled: boolean;
+ /**
+ * Called when select all option is selected
+ */
+ onSelectAll?: () => void;
Review Comment:
Why do you need this event? I expect `onChange` to be called when selecting
all items, just like it would be if I manually selected all items.
--
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]
