fix: Order of Select items when unselecting (#17169)

* fix: Order of Select items when unselecting * Adds a property comparator that supports strings and numbers * Uses a named export for propertyComparator
apache · Oct 25, 2021 · 55be249 · 55be249
1 parent ef3afbd
commit 55be249
Showing 17 changed files with 217 additions and 113 deletions.
diff --git a/superset-frontend/src/addSlice/AddSliceContainer.tsx b/superset-frontend/src/addSlice/AddSliceContainer.tsx
@@ -256,15 +256,11 @@ export default class AddSliceContainer extends React.PureComponent<
         customLabel: ReactNode;
         label: string;
         value: string;
-      }[] = response.json.result
-        .map((item: Dataset) => ({
-          value: `${item.id}__${item.datasource_type}`,
-          customLabel: this.newLabel(item),
-          label: item.table_name,
-        }))
-        .sort((a: { label: string }, b: { label: string }) =>
-          a.label.localeCompare(b.label),
-        );
+      }[] = response.json.result.map((item: Dataset) => ({
+        value: `${item.id}__${item.datasource_type}`,
+        customLabel: this.newLabel(item),
+        label: item.table_name,
+      }));
       return {
         data: list,
         totalCount: response.json.count,

diff --git a/superset-frontend/src/components/DatabaseSelector/index.tsx b/superset-frontend/src/components/DatabaseSelector/index.tsx
@@ -201,22 +201,18 @@ export default function DatabaseSelector({
       // TODO: Would be nice to add pagination in a follow-up. Needs endpoint changes.
       SupersetClient.get({ endpoint })
         .then(({ json }) => {
-          const options = json.result
-            .map((s: string) => ({
-              value: s,
-              label: s,
-              title: s,
-            }))
-            .sort((a: { label: string }, b: { label: string }) =>
-              a.label.localeCompare(b.label),
-            );
+          const options = json.result.map((s: string) => ({
+            value: s,
+            label: s,
+            title: s,
+          }));
           if (onSchemasLoad) {
             onSchemasLoad(options);
           }
           setSchemaOptions(options);
           setLoadingSchemas(false);
         })
-        .catch(e => {
+        .catch(() => {
           setLoadingSchemas(false);
           handleError(t('There was an error loading the schemas'));
         });

diff --git a/superset-frontend/src/components/Select/Select.stories.tsx b/superset-frontend/src/components/Select/Select.stories.tsx
@@ -313,7 +313,7 @@ const USERS = [
   'Claire',
   'Benedetta',
   'Ilenia',
-];
+].sort();
 
 export const AsyncSelect = ({
   fetchOnlyOnSearch,
@@ -429,6 +429,7 @@ export const AsyncSelect = ({
 };
 
 AsyncSelect.args = {
+  allowClear: false,
   allowNewOptions: false,
   fetchOnlyOnSearch: false,
   pageSize: 10,

diff --git a/superset-frontend/src/components/Select/Select.test.tsx b/superset-frontend/src/components/Select/Select.test.tsx
@@ -52,7 +52,7 @@ const OPTIONS = [
   { label: 'Irfan', value: 18, gender: 'Male' },
   { label: 'George', value: 19, gender: 'Male' },
   { label: 'Ashfaq', value: 20, gender: 'Male' },
-];
+].sort((option1, option2) => option1.label.localeCompare(option2.label));
 
 const loadOptions = async (search: string, page: number, pageSize: number) => {
   const totalCount = OPTIONS.length;
@@ -100,6 +100,8 @@ const findSelectValue = () =>
 const findAllSelectValues = () =>
   waitFor(() => getElementsByClassName('.ant-select-selection-item'));
 
+const clearAll = () => userEvent.click(screen.getByLabelText('close-circle'));
+
 const type = (text: string) => {
   const select = getSelect();
   userEvent.clear(select);
@@ -127,6 +129,37 @@ test('inverts the selection', async () => {
   expect(await screen.findByLabelText('stop')).toBeInTheDocument();
 });
 
+test('sort the options by label if no sort comparator is provided', async () => {
+  const unsortedOptions = [...OPTIONS].sort(() => Math.random());
+  render(<Select {...defaultProps} options={unsortedOptions} />);
+  await open();
+  const options = await findAllSelectOptions();
+  options.forEach((option, key) =>
+    expect(option).toHaveTextContent(OPTIONS[key].label),
+  );
+});
+
+test('sort the options using a custom sort comparator', async () => {
+  const sortComparator = (
+    option1: typeof OPTIONS[0],
+    option2: typeof OPTIONS[0],
+  ) => option1.gender.localeCompare(option2.gender);
+  render(
+    <Select
+      {...defaultProps}
+      options={loadOptions}
+      sortComparator={sortComparator}
+    />,
+  );
+  await open();
+  const options = await findAllSelectOptions();
+  const optionsPage = OPTIONS.slice(0, defaultProps.pageSize);
+  const sortedOptions = optionsPage.sort(sortComparator);
+  options.forEach((option, key) =>
+    expect(option).toHaveTextContent(sortedOptions[key].label),
+  );
+});
+
 test('displays the selected values first', async () => {
   render(<Select {...defaultProps} mode="multiple" />);
   const option3 = OPTIONS[2].label;
@@ -141,6 +174,22 @@ test('displays the selected values first', async () => {
   expect(sortedOptions[1]).toHaveTextContent(option8);
 });
 
+test('displays the original order when unselecting', async () => {
+  render(<Select {...defaultProps} mode="multiple" />);
+  const option3 = OPTIONS[2].label;
+  const option8 = OPTIONS[7].label;
+  await open();
+  userEvent.click(await findSelectOption(option3));
+  userEvent.click(await findSelectOption(option8));
+  await type('{esc}');
+  clearAll();
+  await open();
+  const options = await findAllSelectOptions();
+  options.forEach((option, key) =>
+    expect(option).toHaveTextContent(OPTIONS[key].label),
+  );
+});
+
 test('searches for label or value', async () => {
   const option = OPTIONS[11];
   render(<Select {...defaultProps} />);
@@ -172,11 +221,11 @@ test('searches for custom fields', async () => {
   await type('Female');
   options = await findAllSelectOptions();
   expect(options.length).toBe(5);
-  expect(options[0]).toHaveTextContent('Olivia');
-  expect(options[1]).toHaveTextContent('Emma');
-  expect(options[2]).toHaveTextContent('Ava');
-  expect(options[3]).toHaveTextContent('Charlotte');
-  expect(options[4]).toHaveTextContent('Nikole');
+  expect(options[0]).toHaveTextContent('Ava');
+  expect(options[1]).toHaveTextContent('Charlotte');
+  expect(options[2]).toHaveTextContent('Emma');
+  expect(options[3]).toHaveTextContent('Nikole');
+  expect(options[4]).toHaveTextContent('Olivia');
   await type('1');
   expect(screen.getByText(NO_DATA)).toBeInTheDocument();
 });
@@ -227,7 +276,7 @@ test('clear all the values', async () => {
       onClear={onClear}
     />,
   );
-  userEvent.click(screen.getByLabelText('close-circle'));
+  clearAll();
   expect(onClear).toHaveBeenCalled();
   const values = await findAllSelectValues();
   expect(values.length).toBe(0);
@@ -378,8 +427,8 @@ test('static - searches for an item', async () => {
   await type(search);
   const options = await findAllSelectOptions();
   expect(options.length).toBe(2);
-  expect(options[0]).toHaveTextContent('Olivia');
-  expect(options[1]).toHaveTextContent('Oliver');
+  expect(options[0]).toHaveTextContent('Oliver');
+  expect(options[1]).toHaveTextContent('Olivia');
 });
 
 test('async - renders the select with default props', () => {
@@ -546,8 +595,8 @@ test('async - searches for an item already loaded', async () => {
   await waitForElementToBeRemoved(screen.getByText(LOADING));
   const options = await findAllSelectOptions();
   expect(options.length).toBe(2);
-  expect(options[0]).toHaveTextContent('Olivia');
-  expect(options[1]).toHaveTextContent('Oliver');
+  expect(options[0]).toHaveTextContent('Oliver');
+  expect(options[1]).toHaveTextContent('Olivia');
 });
 
 test('async - searches for an item in a page not loaded', async () => {
@@ -599,7 +648,7 @@ test('async - does not fire a new request for the same search input', async () =
   await type('search');
   expect(await screen.findByText(NO_DATA)).toBeInTheDocument();
   expect(loadOptions).toHaveBeenCalledTimes(1);
-  userEvent.click(screen.getByLabelText('close-circle'));
+  clearAll();
   await type('search');
   expect(await screen.findByText(NO_DATA)).toBeInTheDocument();
   expect(loadOptions).toHaveBeenCalledTimes(1);

diff --git a/superset-frontend/src/components/Select/Select.tsx b/superset-frontend/src/components/Select/Select.tsx
@@ -94,6 +94,7 @@ export interface SelectProps extends PickedSelectProps {
   invertSelection?: boolean;
   fetchOnlyOnSearch?: boolean;
   onError?: (error: string) => void;
+  sortComparator?: (a: AntdLabeledValue, b: AntdLabeledValue) => number;
 }
 
 const StyledContainer = styled.div`
@@ -168,6 +169,30 @@ const Error = ({ error }: { error: string }) => (
   </StyledError>
 );
 
+const defaultSortComparator = (a: AntdLabeledValue, b: AntdLabeledValue) => {
+  if (typeof a.label === 'string' && typeof b.label === 'string') {
+    return a.label.localeCompare(b.label);
+  }
+  if (typeof a.value === 'string' && typeof b.value === 'string') {
+    return a.value.localeCompare(b.value);
+  }
+  return (a.value as number) - (b.value as number);
+};
+
+/**
+ * It creates a comparator to check for a specific property.
+ * Can be used with string and number property values.
+ * */
+export const propertyComparator = (property: string) => (
+  a: AntdLabeledValue,
+  b: AntdLabeledValue,
+) => {
+  if (typeof a[property] === 'string' && typeof b[property] === 'string') {
+    return a[property].localeCompare(b[property]);
+  }
+  return (a[property] as number) - (b[property] as number);
+};
+
 const Select = ({
   allowNewOptions = false,
   ariaLabel,
@@ -189,6 +214,7 @@ const Select = ({
   pageSize = DEFAULT_PAGE_SIZE,
   placeholder = t('Select ...'),
   showSearch = true,
+  sortComparator = defaultSortComparator,
   value,
   ...props
 }: SelectProps) => {
@@ -277,14 +303,21 @@ const Select = ({
             }
           });
         }
-
-        const sortedOptions = [...topOptions, ...otherOptions];
+        const sortedOptions = [
+          ...topOptions.sort(sortComparator),
+          ...otherOptions.sort(sortComparator),
+        ];
+        if (!isEqual(sortedOptions, selectOptions)) {
+          setSelectOptions(sortedOptions);
+        }
+      } else {
+        const sortedOptions = [...selectOptions].sort(sortComparator);
         if (!isEqual(sortedOptions, selectOptions)) {
           setSelectOptions(sortedOptions);
         }
       }
     },
-    [isAsync, isSingleMode, labelInValue, selectOptions],
+    [isAsync, isSingleMode, labelInValue, selectOptions, sortComparator],
   );
 
   const handleOnSelect = (
@@ -342,28 +375,34 @@ const Select = ({
     [onError],
   );
 
-  const handleData = (data: OptionsType) => {
-    let mergedData: OptionsType = [];
-    if (data && Array.isArray(data) && data.length) {
-      const dataValues = new Set();
-      data.forEach(option =>
-        dataValues.add(String(option.value).toLocaleLowerCase()),
-      );
-
-      // merges with existing and creates unique options
-      setSelectOptions(prevOptions => {
-        mergedData = [
-          ...prevOptions.filter(
-            previousOption =>
-              !dataValues.has(String(previousOption.value).toLocaleLowerCase()),
-          ),
-          ...data,
-        ];
-        return mergedData;
-      });
-    }
-    return mergedData;
-  };
+  const handleData = useCallback(
+    (data: OptionsType) => {
+      let mergedData: OptionsType = [];
+      if (data && Array.isArray(data) && data.length) {
+        const dataValues = new Set();
+        data.forEach(option =>
+          dataValues.add(String(option.value).toLocaleLowerCase()),
+        );
+
+        // merges with existing and creates unique options
+        setSelectOptions(prevOptions => {
+          mergedData = [
+            ...prevOptions.filter(
+              previousOption =>
+                !dataValues.has(
+                  String(previousOption.value).toLocaleLowerCase(),
+                ),
+            ),
+            ...data,
+          ];
+          mergedData.sort(sortComparator);
+          return mergedData;
+        });
+      }
+      return mergedData;
+    },
+    [sortComparator],
+  );
 
   const handlePaginatedFetch = useMemo(
     () => (value: string, page: number, pageSize: number) => {
@@ -401,7 +440,7 @@ const Select = ({
           setIsTyping(false);
         });
     },
-    [allValuesLoaded, fetchOnlyOnSearch, internalOnError, options],
+    [allValuesLoaded, fetchOnlyOnSearch, handleData, internalOnError, options],
   );
 
   const handleOnSearch = useMemo(
@@ -474,8 +513,8 @@ const Select = ({
     }
 
     // multiple or tags mode keep the dropdown visible while selecting options
-    // this waits for the dropdown to be closed before sorting the top options
-    if (!isSingleMode && !isDropdownVisible) {
+    // this waits for the dropdown to be opened before sorting the top options
+    if (!isSingleMode && isDropdownVisible) {
       handleTopOptions(selectValue);
     }
   };

diff --git a/superset-frontend/src/components/TableSelector/index.tsx b/superset-frontend/src/components/TableSelector/index.tsx
@@ -209,11 +209,7 @@ const TableSelector: FunctionComponent<TableSelectorProps> = ({
           if (onTablesLoad) {
             onTablesLoad(json.options);
           }
-          setTableOptions(
-            options.sort((a: { text: string }, b: { text: string }) =>
-              a.text.localeCompare(b.text),
-            ),
-          );
+          setTableOptions(options);
           setCurrentTable(currentTable);
           setLoadingTables(false);
         })

diff --git a/superset-frontend/src/components/TimezoneSelector/index.tsx b/superset-frontend/src/components/TimezoneSelector/index.tsx
@@ -83,17 +83,13 @@ ALL_ZONES.forEach(zone => {
   }
 });
 
-const TIMEZONE_OPTIONS = TIMEZONES.sort(
-  // sort by offset
-  (a, b) =>
-    moment.tz(currentDate, a.name).utcOffset() -
-    moment.tz(currentDate, b.name).utcOffset(),
-).map(zone => ({
+const TIMEZONE_OPTIONS = TIMEZONES.map(zone => ({
   label: `GMT ${moment
     .tz(currentDate, zone.name)
     .format('Z')} (${getTimezoneName(zone.name)})`,
   value: zone.name,
   offsets: getOffsetKey(zone.name),
+  timezoneName: zone.name,
 }));
 
 const TimezoneSelector = ({ onTimezoneChange, timezone }: TimezoneProps) => {
@@ -126,6 +122,13 @@ const TimezoneSelector = ({ onTimezoneChange, timezone }: TimezoneProps) => {
       onChange={onTimezoneChange}
       value={timezone || DEFAULT_TIMEZONE.value}
       options={TIMEZONE_OPTIONS}
+      sortComparator={(
+        a: typeof TIMEZONE_OPTIONS[number],
+        b: typeof TIMEZONE_OPTIONS[number],
+      ) =>
+        moment.tz(currentDate, a.timezoneName).utcOffset() -
+        moment.tz(currentDate, b.timezoneName).utcOffset()
+      }
     />
   );
 };