feat(select): sort exact and startsWith match to first #18856

ktmud · 2022-02-22T21:47:30Z

SUMMARY

Sort exact match and startsWith matches in the Select component to top of the list. This improves the user experience when searching a very large list where a partial match may be sorted to the top even when an extract match exists only because the former ranks higher in lexicographic order.

#16414 tried to tackle this but the work was put on hold as the Select component was undergoing a significant migration and feature-enrichment. Now that it's more stable, it's worth giving this problem another go.

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before

Partial match may be sorted to the top if it ranks higher alphabetically:

After

"Starts with" and exact matches always ranks higher, making it easier to search for and select things the users actually want:

TESTING INSTRUCTIONS

Go to any Select component and search for things

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

codecov · 2022-03-04T01:41:54Z

Codecov Report

Merging #18856 (1496be1) into master (3298551) will decrease coverage by 0.02%.
The diff coverage is 89.23%.

@@            Coverage Diff             @@
##           master   #18856      +/-   ##
==========================================
- Coverage   66.52%   66.50%   -0.03%     
==========================================
  Files        1641     1642       +1     
  Lines       63475    63442      -33     
  Branches     6443     6439       -4     
==========================================
- Hits        42226    42189      -37     
+ Misses      19584    19583       -1     
- Partials     1665     1670       +5

Flag	Coverage Δ
javascript	`51.22% <89.23%> (-0.07%)`	⬇️

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

Impacted Files	Coverage Δ
superset-frontend/src/utils/rankedSearchCompare.ts	`33.33% <33.33%> (ø)`
superset-frontend/src/components/Select/Select.tsx	`85.21% <94.64%> (-1.58%)`	⬇️
superset-frontend/src/components/Select/utils.ts	`52.94% <100.00%> (-6.15%)`	⬇️
...c/filters/components/Select/SelectFilterPlugin.tsx	`65.78% <0.00%> (-2.64%)`	⬇️
superset-frontend/src/chart/Chart.jsx	`53.33% <0.00%> (-0.12%)`	⬇️
...frontend/src/views/CRUD/alert/AlertReportModal.tsx	`52.38% <0.00%> (+0.14%)`	⬆️

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 3298551...1496be1. Read the comment docs.

ktmud · 2022-03-04T02:30:28Z

@michael-s-molina @geido this is ready for review.

cc @etr2460 since you've also worked on this area before.

ktmud · 2022-03-04T05:10:47Z

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

-  const allowFetch = !fetchOnlyOnSearch || searchedValue;
-
-  // TODO: Don't assume that isAsync is always labelInValue
-  const handleTopOptions = useCallback(


handleTopOptions is not needed anymore since we are handling sorting selected options to the top also in the sort comparator.

superset-frontend/src/utils/rankedSearchCompare.ts

ktmud · 2022-03-04T05:24:18Z

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

@@ -577,7 +538,6 @@ const Select = ({

    if (filterOption) {
      const searchValue = search.trim().toLowerCase();


We should probably remove trimming and case insensitivity treatment at this place since whitespaces and cases can be useful in search and ranking, too. This is especially important if we are to pass the search query to some more sophisticated backend search engine. For example, search "Le" may rank "Leah" first, but "Le " (with a space at the end) should rank things like "Le Monde" first.

That's an interesting point. We also need to consider frequent use cases. If I search for "michael", I expect to get "Michael". We can tackle this in more depth when adding support to sophisticated backend search engines.

michael-s-molina

Thank you for the contribution @ktmud. I left some comments.

I also found a problem when allowNewOptions is true. If you type a lower case text that has a match but it hasn't being retrieved from the server-side yet, then a new option is created. As soon as the value is loaded on the client-side, the behavior changes.

Screen.Recording.2022-03-04.at.11.15.47.AM.mov

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

superset-frontend/src/utils/rankedSearchCompare.ts

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

michael-s-molina · 2022-03-04T14:14:20Z

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

@@ -577,7 +538,6 @@ const Select = ({

    if (filterOption) {
      const searchValue = search.trim().toLowerCase();


That's an interesting point. We also need to consider frequent use cases. If I search for "michael", I expect to get "Michael". We can tackle this in more depth when adding support to sophisticated backend search engines.

ktmud · 2022-03-04T22:07:13Z

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

@@ -153,11 +153,12 @@ describe('SelectControl', () => {
    });

    describe('when select has a sortComparator prop', () => {
-      it('does not add add order key and sorts by sortComparator', () => {


This test case expect the child component Select to update a prop passed down from SelectComponent, which is a bad practice as all React props should be immutable.

I didn't see anything breaking after removing this behavior. @corbinrobb @michael-s-molina do you remember why this was needed?

ktmud · 2022-03-04T22:13:54Z

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

-    [isAsync, isSingleMode, labelInValue, selectOptions, sortComparator],
+  const allowFetch = !fetchOnlyOnSearch || inputValue;
+
+  const sortSelectedFirst = useCallback(


We should probably provide an option to disable this behavior because in smaller more static lists (e.g. the Row Limit select), it's more predictable when the select options are fixed. There is no cognitive load in "where is the other option I just saw near the option I selected".

We had this discussion previously, and we opted for consistency. When we were studying the old Select, one thing we noticed is that it contained a lot of configuration properties and that resulted in increased complexity and many different behaviors across the application. The user was never sure how the component would behave on each particular screen. One of our main objectives with the new component was to reduce the number of configuration properties, making conscious defaults to reduce complexity and standardize behavior. That's why we don't expose all Ant Design Select properties for example.

I'm not even sure why we added this behavior in the first place. Neither antd Select or react-select or the native select do this, actually making this behavior inconsistent with a select component users see in other places.

If this is about making multi-select more usable, then multi select should have used tags mode anyway.

This behavior is quite common actually. You can see the same pattern on Github. This was also an input from the design team that considered it useful for the users. If you need more information about this you can check the original request at #14842

If this is about making multi-select more usable, then multi-select should have used tags mode anyway.

We do use the tags mode for multi-select. We consider this an extended behavior.

This behavior only makes sense for multi-select or async select where options are not available on the first page, because for single select you can always just scroll to the selected value when menu opens. The github example is also a multi-select. I'd consider a full scale multi-select with async searches a totally different experience than single select with pre-defined values therefore a little bit "inconsistency" should be warranted.

geido · 2022-03-07T11:46:05Z

superset-frontend/src/components/Select/utils.ts

+    optionsArray.find(
+      x =>
+        x === value ||
+        (typeof x === 'object' &&
+          (('value' in x && x.value === value) ||
+            (checkLabel && 'label' in x && x.label === value))),


Just a nit but I think it's better for readability

Suggested change

optionsArray.find(

x =>

x === value ||

(typeof x === 'object' &&

(('value' in x && x.value === value) ||

(checkLabel && 'label' in x && x.label === value))),

optionsArray.find(

option =>

option === value ||

(typeof option === 'object' &&

(('value' in option && option.value === value) ||

(checkLabel && 'label' in option && option.label === value))),

See my comment in #18799 (comment)

geido · 2022-03-07T11:48:16Z

/testenv up

github-actions · 2022-03-07T11:50:09Z

@geido Ephemeral environment spinning up at http://54.185.166.79:8080. Credentials are admin/admin. Please allow several minutes for bootstrapping and startup.

geido · 2022-03-07T17:51:40Z

Hey @ktmud FYI we had a quick session with @michael-s-molina and noticed that currently on master the options start to get filtered only after typing the second character. If you only type one character, the options are sorted alphabetically. We think this is a problem as the behavior is inconsistent. We probably should apply the filtering starting from the very first typed character. Let me know if you want to tackle this problem here or if we should take it to a follow-up. Thanks!

ktmud · 2022-03-07T20:07:26Z

@michael-s-molina I can't reproduce what you described in both master and my branch. Both branches start to filter results after the first character typed. It's just the ordering is different.

master branch

my branch (this PR)

Looking at the code, I also don't see why would 1 character vs more characters will filter differently.

If this is a real problem but unrelated to my changes, I'd prefer to see it addressed in another PR.

michael-s-molina · 2022-03-08T12:16:55Z

Hey @ktmud FYI we had a quick session with @michael-s-molina and noticed that currently on master the options start to get filtered only after typing the second character. If you only type one character, the options are sorted alphabetically. We think this is a problem as the behavior is inconsistent. We probably should apply the filtering starting from the very first typed character. Let me know if you want to tackle this problem here or if we should take it to a follow-up. Thanks!

If this is a real problem but unrelated to my changes, I'd prefer to see it addressed in another PR.

@ktmud @geido The problem is not related to the PR changes. We'll handle it in another PR.

michael-s-molina

LGTM

geido

LGTM!

github-actions · 2022-03-08T15:09:12Z

Ephemeral environment shutdown and build artifacts deleted.

(cherry picked from commit c75f233)

ktmud requested review from michael-s-molina and geido as code owners February 22, 2022 21:47

pull-request-size bot added the size/L label Feb 22, 2022

ktmud mentioned this pull request Feb 22, 2022

fix: Add case sensitive option to SelectControl #18837

Closed

9 tasks

ktmud force-pushed the dataset-select-search-order branch from 6ef5aed to 4edf089 Compare February 23, 2022 02:39

ktmud force-pushed the dataset-select-search-order branch 2 times, most recently from 8de112c to 8cb01af Compare March 4, 2022 00:59

ktmud force-pushed the dataset-select-search-order branch 2 times, most recently from aa93ba6 to b216b3b Compare March 4, 2022 02:30

ktmud force-pushed the dataset-select-search-order branch 4 times, most recently from c3a0a4d to 144d79a Compare March 4, 2022 05:08

ktmud commented Mar 4, 2022

View reviewed changes

ktmud force-pushed the dataset-select-search-order branch from 1a1881f to d01a32e Compare March 4, 2022 05:26

michael-s-molina requested changes Mar 4, 2022

View reviewed changes

ktmud added 3 commits March 4, 2022 09:45

feat(select): sort exact and startsWith match to first

0719f3c

More test case

083925d

Address PR comments

eea20ee

ktmud force-pushed the dataset-select-search-order branch from 9c8a546 to eea20ee Compare March 4, 2022 17:45

pull-request-size bot added size/XL and removed size/L labels Mar 4, 2022

clean up

e706228

ktmud force-pushed the dataset-select-search-order branch from 48d1668 to e706228 Compare March 4, 2022 18:42

ktmud added 2 commits March 4, 2022 11:55

check options

e5fbd09

Clean up sortComparator test case

1496be1

ktmud commented Mar 4, 2022

View reviewed changes

geido reviewed Mar 7, 2022

View reviewed changes

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

View reviewed changes

geido approved these changes Mar 8, 2022

View reviewed changes

ktmud merged commit c75f233 into apache:master Mar 8, 2022

ktmud deleted the dataset-select-search-order branch March 8, 2022 17:01

ktmud mentioned this pull request Mar 9, 2022

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

Merged

9 tasks

villebro added the lts-v1 label Mar 29, 2022

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

feat(select): sort exact and startsWith match to first (#18856)

032f560

(cherry picked from commit c75f233)

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): sort exact and startsWith match to first #18856

feat(select): sort exact and startsWith match to first #18856

ktmud commented Feb 22, 2022 •

edited

Loading

codecov bot commented Mar 4, 2022 •

edited

Loading

ktmud commented Mar 4, 2022

ktmud Mar 4, 2022

ktmud Mar 4, 2022 •

edited

Loading

michael-s-molina Mar 4, 2022

michael-s-molina left a comment

michael-s-molina Mar 4, 2022

ktmud Mar 4, 2022

ktmud Mar 4, 2022

michael-s-molina Mar 7, 2022

ktmud Mar 7, 2022

michael-s-molina Mar 7, 2022 •

edited

Loading

ktmud Mar 7, 2022 •

edited

Loading

geido Mar 7, 2022

ktmud Mar 7, 2022

geido commented Mar 7, 2022

github-actions bot commented Mar 7, 2022

geido commented Mar 7, 2022

ktmud commented Mar 7, 2022

michael-s-molina commented Mar 8, 2022

michael-s-molina left a comment

geido left a comment

github-actions bot commented Mar 8, 2022

		@@ -577,7 +538,6 @@ const Select = ({

		if (filterOption) {
		const searchValue = search.trim().toLowerCase();

feat(select): sort exact and startsWith match to first #18856

feat(select): sort exact and startsWith match to first #18856

Conversation

ktmud commented Feb 22, 2022 • edited Loading

SUMMARY

BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF

Before

After

TESTING INSTRUCTIONS

ADDITIONAL INFORMATION

codecov bot commented Mar 4, 2022 • edited Loading

Codecov Report

ktmud commented Mar 4, 2022

Choose a reason for hiding this comment

ktmud Mar 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michael-s-molina left a 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 Mar 7, 2022 • edited Loading

Choose a reason for hiding this comment

ktmud Mar 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

geido commented Mar 7, 2022

github-actions bot commented Mar 7, 2022

geido commented Mar 7, 2022

ktmud commented Mar 7, 2022

master branch

my branch (this PR)

michael-s-molina commented Mar 8, 2022

michael-s-molina left a comment

Choose a reason for hiding this comment

geido left a comment

Choose a reason for hiding this comment

github-actions bot commented Mar 8, 2022

ktmud commented Feb 22, 2022 •

edited

Loading

codecov bot commented Mar 4, 2022 •

edited

Loading

ktmud Mar 4, 2022 •

edited

Loading

michael-s-molina Mar 7, 2022 •

edited

Loading

ktmud Mar 7, 2022 •

edited

Loading