feat(select): keep options order when not in async mode #19085

ktmud · 2022-03-09T18:16:39Z

SUMMARY

After applying a more sensible search sort comparator in #18856 , I realized that the new sorting isn't applied to non-async select options in Explore controls. This is because they are using a default sort comparator added to Explore's Select control in #17638, which was trying to address the issue where options are unnecessarily sorted by labels for numeric values.

In reality, we can get away with the problem raised in #17638 by simply not messing with the input option orders when users are not searching. The options have two sorted orders:

When users are not searching, they should be arranged in their logical order---alphabetical for strings and order by value for numbers, or keeping the original orders in the table schema for table columns. This is better managed by options providers themselves (either a parent component or an async API).
When users are searching, we should always sort the options based on a search matching score on option labels. For numbers, as long as we sort startsWith match to the top, they will be correctly sorted even if we sort by label strings.

By preserving the original order of input options, we avoid having to add custom sorters in most cases. When users are not searching, we just use whatever orders options provider pass in; when users are searching, we apply the same match sort algorithm for maximum consistency. Sorting the unfiltered options list only makes sense when merge async results from different search queries or multiple pages where the orders of the options in the merged list aren't guaranteed. For this case, we can introduce a mergeData config function to indicate how data should be merged in the frontend---the order of the merged data will still be preserved when the search string is empty.

This PR changes the Select component to not reorder the predefined options list when in non-async mode, which is also the original behavior of the now deprecated Select component based on react-select.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before

Search sort exact match and starts with first isn't applied to countries select in Country Map

Numeric values are correctly sorted and selected values will be pushed to the top:

After

Countries can be filtered with the improved sort:

Numbers are still correctly sorted and selected values will not be pushed to the top:

Multi select will still sort selected values to the top (same as before):

TESTING INSTRUCTIONS

Test the select controls in different chart types

ADDITIONAL INFORMATION

Has associated issue:
Required feature flags:
Changes UI
Includes DB Migration (follow approval process in SIP-59)
- Migration is atomic, supports rollback & is backwards-compatible
- Confirm DB migration upgrade and downgrade tested
- Runtime estimates and downtime expectations provided
Introduces new feature or API
Removes existing feature or API

michael-s-molina · 2022-03-09T18:57:17Z

@ktmud Is it possible to fix the Explore issue first and leave the discussion of changing Select order to a follow-up PR? We'll probably need design input about this and I fear the discussion may delay the bug fix.

ktmud · 2022-03-09T19:19:23Z

@michael-s-molina I'd be happy to wait and hear counter arguments for this change. Fixing just the Explore controls may lead to even more complexity which I'd be hesitant to dive into.

ktmud · 2022-03-09T20:02:49Z

Another thing to call out is #17638 also changed the columns select to always sort table columns alphabetically, but this may not be desired because table schemas normally already order columns in some specific order that makes the most sense to the table.

This PR reverts this behavior so that users always see columns in the order as they appear in table schema.

Before this PR

For the "birth_names" dataset:

After this PR and before #17638

codecov · 2022-03-10T01:48:29Z

Codecov Report

Merging #19085 (5ed22f8) into master (7524e1e) will increase coverage by 0.00%.
The diff coverage is 90.00%.

❗ Current head 5ed22f8 differs from pull request most recent head 4c0bc70. Consider uploading reports for the commit 4c0bc70 to get more accurate results

@@           Coverage Diff           @@
##           master   #19085   +/-   ##
=======================================
  Coverage   66.52%   66.52%           
=======================================
  Files        1646     1646           
  Lines       63540    63538    -2     
  Branches     6466     6465    -1     
=======================================
+ Hits        42268    42270    +2     
+ Misses      19596    19595    -1     
+ Partials     1676     1673    -3

Flag	Coverage Δ
javascript	`51.28% <90.00%> (+<0.01%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...et-ui-chart-controls/src/shared-controls/index.tsx	`38.00% <ø> (+0.37%)`	⬆️
...nd/plugins/plugin-chart-table/src/controlPanel.tsx	`16.66% <ø> (+0.49%)`	⬆️
...ins/plugin-chart-table/src/utils/isEqualColumns.ts	`100.00% <ø> (ø)`
...frontend/src/components/TimezoneSelector/index.tsx	`96.96% <ø> (ø)`
...end/src/dashboard/util/filterboxMigrationHelper.ts	`38.88% <0.00%> (-0.32%)`	⬇️
...onalFormattingControl/FormattingPopoverContent.tsx	`54.28% <ø> (ø)`
...nts/controls/DateFilterControl/DateFilterLabel.tsx	`41.83% <ø> (ø)`
...trols/DateFilterControl/components/CustomFrame.tsx	`72.41% <ø> (-0.92%)`	⬇️
...ore/components/controls/DateFilterControl/types.ts	`100.00% <ø> (ø)`
...l/AdhocFilterEditPopoverSimpleTabContent/index.tsx	`65.60% <ø> (+0.52%)`	⬆️
... and 7 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7524e1e...4c0bc70. Read the comment docs.

ktmud · 2022-03-10T03:31:00Z

@michael-s-molina I managed to keep the sorting selected to top behavior while achieving the main goal of this PR--preserving the input order of predefined options list so that we don't have to provide custom sortComparator but rather reuse the default match sorter instead. I've also updated the PR description accordingly. Can you PTAL? Thanks!

ktmud · 2022-03-10T03:32:29Z

superset-frontend/packages/superset-ui-core/src/utils/isEqualArray.test.ts

+ * specific language governing permissions and limitations
+ * under the License.
+ */
+import isEqualArray from './isEqualArray';


Basically a faster version of lodash.isEqual where we only compare items in arrays shallowly. Also used in Table chart.

ktmud · 2022-03-10T03:34:04Z

superset-frontend/src/components/TimezoneSelector/index.tsx

+  moment.tz(currentDate, b.timezoneName).utcOffset();
+
+// pre-sort timezone options
+TIMEZONE_OPTIONS.sort(TIMEZONE_OPTIONS_SORT_COMPARTOR);


Select control now expects options to be pre-sorted.

ktmud · 2022-03-10T09:13:06Z

superset-frontend/packages/superset-ui-core/src/utils/isEqualArray.ts

@@ -23,9 +23,11 @@ export default function isEqualArray<T extends unknown[] | undefined | null>(
  return (
    arrA === arrB ||
    (!arrA && !arrB) ||
-    (arrA &&
+    !!(


Making sure return result is always boolean

ktmud · 2022-03-10T09:14:59Z

superset-frontend/src/explore/components/controls/SelectControl.jsx

-      nextProps.choices !== this.props.choices ||
-      nextProps.options !== this.props.options
+      !isEqualArray(nextProps.choices, this.props.choices) ||
+      !isEqualArray(nextProps.options, this.props.options)


Had to run this deeper check because mapStateToProps may create columns list with a new array but same content. We may need to dig deeper to those excessive rerendering and check whether they are actually necessary.

ktmud · 2022-03-10T09:18:18Z

superset-frontend/src/explore/components/controls/SelectControl.test.jsx

@@ -37,9 +37,9 @@ const defaultProps = {
 };

 const options = [
-  { value: '1 year ago', label: '1 year ago', order: 0 },
-  { value: '1 week ago', label: '1 week ago', order: 1 },
-  { value: 'today', label: 'today', order: 2 },


May do a bigger sweep in other components to clean up similar order props later.

michael-s-molina · 2022-03-10T14:36:41Z

superset-frontend/src/components/Select/Select.test.tsx

 });

-test('displays the original order when unselecting', async () => {
+// eslint-disable-next-line jest/expect-expect


May I suggest renaming checkOrder to matchOrder, and returning a boolean? This way we could have the expect clauses inside the tests and avoid disabling lint rules.

good suggestion!

michael-s-molina · 2022-03-10T14:37:11Z

superset-frontend/src/components/Select/Select.test.tsx

-  userEvent.click(await findSelectOption(option3));
-  userEvent.click(await findSelectOption(option8));
+  userEvent.click(await findSelectOption(originalLabels[1]));
+  // after selection, keep the original order


Nice improvement to the test!

michael-s-molina · 2022-03-10T15:01:18Z

superset-frontend/src/components/Select/Select.tsx

+      sortSelectedFirst(a, b) ||
+      // Only apply the custom sorter in async mode because we should
+      // preserve the options order as much as possible.
+      (isAsync ? sortComparator(a, b, '') : 0),


Can we add the following docs (or something similar) to sortComparator?

export interface SelectProps extends PickedSelectProps { ... /** * Custom sorting comparator for the options. * Works in async mode only. */ sortComparator?: typeof DEFAULT_SORT_COMPARATOR;

superset-frontend/src/components/Select/Select.tsx

michael-s-molina · 2022-03-10T16:18:27Z

May do a bigger sweep in other components to clean up similar order props later.

If you check the references to sortComparator, we have only 10 files. It would be a good idea to clean these as part of this PR to avoid confusion on master since this property won't be working for these selects. It's also important to validate sorting for these use cases, especially the SelectFilterPlugin where the order can be reversed.

michael-s-molina · 2022-03-10T16:22:24Z

@ktmud Thanks for clearly documenting changes in code or comments. This helps a lot with the review and code readability. I'll try to execute some tests tomorrow to make sure we didn't break anything.

ktmud · 2022-03-10T16:58:26Z

@michael-s-molina I cleaned up all references of propertyComparator('order') and left other sort comparators in place.

michael-s-molina · 2022-03-11T13:21:50Z

This PR reverts this behavior so that users always see columns in the order as they appear in table schema.

I think this is a reasonable change and it's following the same pattern of the Columns left menu.

michael-s-molina

LGTM. Thanks for the improvements!

(cherry picked from commit ae13d83)

ktmud requested review from michael-s-molina and geido as code owners March 9, 2022 18:16

pull-request-size bot added the size/L label Mar 9, 2022

ktmud requested a review from villebro March 9, 2022 18:24

ktmud force-pushed the select-sort-rank-sweep branch from c6aa009 to 30b3d2b Compare March 9, 2022 19:02

ktmud force-pushed the select-sort-rank-sweep branch 2 times, most recently from 15c7d22 to 5ff71e9 Compare March 9, 2022 19:40

ktmud force-pushed the select-sort-rank-sweep branch 4 times, most recently from abf20fb to 46a9d0e Compare March 10, 2022 01:13

ktmud mentioned this pull request Mar 10, 2022

refactor(TimezoneSelector): simplify override logics and tests #19090

Merged

9 tasks

ktmud force-pushed the select-sort-rank-sweep branch 2 times, most recently from a55e4ff to 24616b2 Compare March 10, 2022 03:18

ktmud commented Mar 10, 2022

View reviewed changes

michael-s-molina reviewed Mar 10, 2022

View reviewed changes

superset-frontend/src/components/Select/Select.tsx Outdated Show resolved Hide resolved

ktmud force-pushed the select-sort-rank-sweep branch 2 times, most recently from e4959f4 to 17a1363 Compare March 10, 2022 18:21

michael-s-molina approved these changes Mar 11, 2022

View reviewed changes

feat(select): keep options order when in single mode

4c0bc70

ktmud force-pushed the select-sort-rank-sweep branch from 17a1363 to 4c0bc70 Compare March 11, 2022 19:55

ktmud merged commit ae13d83 into apache:master Mar 11, 2022

ktmud deleted the select-sort-rank-sweep branch March 11, 2022 21:01

villebro added the lts-v1 label Mar 29, 2022

villebro pushed a commit that referenced this pull request Apr 3, 2022

feat(select): keep options order when in single mode (#19085)

fcca027

(cherry picked from commit ae13d83)

mistercrunch added 🍒 1.5.0 🍒 1.5.1 🍒 1.5.2 🍒 1.5.3 🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels 🚢 2.0.0 labels Mar 13, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(select): keep options order when not in async mode #19085

feat(select): keep options order when not in async mode #19085

ktmud commented Mar 9, 2022 •

edited

Loading

michael-s-molina commented Mar 9, 2022

ktmud commented Mar 9, 2022 •

edited

Loading

ktmud commented Mar 9, 2022 •

edited

Loading

codecov bot commented Mar 10, 2022 •

edited

Loading

ktmud commented Mar 10, 2022 •

edited

Loading

ktmud Mar 10, 2022

ktmud Mar 10, 2022

ktmud Mar 10, 2022

ktmud Mar 10, 2022

ktmud Mar 10, 2022

michael-s-molina Mar 10, 2022

ktmud Mar 10, 2022

michael-s-molina Mar 10, 2022

michael-s-molina Mar 10, 2022

michael-s-molina commented Mar 10, 2022 •

edited

Loading

michael-s-molina commented Mar 10, 2022

ktmud commented Mar 10, 2022 •

edited

Loading

michael-s-molina commented Mar 11, 2022

michael-s-molina left a comment

feat(select): keep options order when not in async mode #19085

feat(select): keep options order when not in async mode #19085

Conversation

ktmud commented Mar 9, 2022 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before

After

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

michael-s-molina commented Mar 9, 2022

ktmud commented Mar 9, 2022 • edited Loading

ktmud commented Mar 9, 2022 • edited Loading

Before this PR

After this PR and before #17638

codecov bot commented Mar 10, 2022 • edited Loading

Codecov Report

ktmud commented Mar 10, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michael-s-molina commented Mar 10, 2022 • edited Loading

michael-s-molina commented Mar 10, 2022

ktmud commented Mar 10, 2022 • edited Loading

michael-s-molina commented Mar 11, 2022

michael-s-molina left a comment

Choose a reason for hiding this comment

ktmud commented Mar 9, 2022 •

edited

Loading

ktmud commented Mar 9, 2022 •

edited

Loading

ktmud commented Mar 9, 2022 •

edited

Loading

codecov bot commented Mar 10, 2022 •

edited

Loading

ktmud commented Mar 10, 2022 •

edited

Loading

michael-s-molina commented Mar 10, 2022 •

edited

Loading

ktmud commented Mar 10, 2022 •

edited

Loading