[GitHub] [superset] michael-s-molina commented on a diff in pull request #22084: feat: Select all for synchronous select

[GitBox <gi...@apache.org>]

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: notifications-unsubscribe@superset.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: notifications-unsubscribe@superset.apache.org
For additional commands, e-mail: notifications-help@superset.apache.org