Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 6 commits into from
Mar 8, 2022

Conversation

ktmud
Copy link
Member

@ktmud ktmud commented Feb 22, 2022

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:

Xnip2022-02-22_13-26-49

After

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

Xnip2022-02-22_13-27-09

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

@ktmud ktmud force-pushed the dataset-select-search-order branch from 6ef5aed to 4edf089 Compare February 23, 2022 02:39
@ktmud ktmud force-pushed the dataset-select-search-order branch 2 times, most recently from 8de112c to 8cb01af Compare March 4, 2022 00:59
@codecov
Copy link

codecov bot commented Mar 4, 2022

Codecov Report

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

Impacted file tree graph

@@            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 ktmud force-pushed the dataset-select-search-order branch 2 times, most recently from aa93ba6 to b216b3b Compare March 4, 2022 02:30
@ktmud
Copy link
Member Author

ktmud commented Mar 4, 2022

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

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

@ktmud ktmud force-pushed the dataset-select-search-order branch 4 times, most recently from c3a0a4d to 144d79a Compare March 4, 2022 05:08
const allowFetch = !fetchOnlyOnSearch || searchedValue;

// TODO: Don't assume that isAsync is always labelInValue
const handleTopOptions = useCallback(
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

if (filterOption) {
const searchValue = search.trim().toLowerCase();
Copy link
Member Author

@ktmud ktmud Mar 4, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ktmud force-pushed the dataset-select-search-order branch from 1a1881f to d01a32e Compare March 4, 2022 05:26
Copy link
Member

@michael-s-molina michael-s-molina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
superset-frontend/src/components/Select/Select.tsx Outdated Show resolved Hide resolved
@@ -577,7 +538,6 @@ const Select = ({

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ktmud force-pushed the dataset-select-search-order branch from 9c8a546 to eea20ee Compare March 4, 2022 17:45
@pull-request-size pull-request-size bot added size/XL and removed size/L labels Mar 4, 2022
@ktmud ktmud force-pushed the dataset-select-search-order branch from 48d1668 to e706228 Compare March 4, 2022 18:42
@@ -153,11 +153,12 @@ describe('SelectControl', () => {
});

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

[isAsync, isSingleMode, labelInValue, selectOptions, sortComparator],
const allowFetch = !fetchOnlyOnSearch || inputValue;

const sortSelectedFirst = useCallback(
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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".

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

@michael-s-molina michael-s-molina Mar 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

@ktmud ktmud Mar 7, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment on lines +76 to +81
optionsArray.find(
x =>
x === value ||
(typeof x === 'object' &&
(('value' in x && x.value === value) ||
(checkLabel && 'label' in x && x.label === value))),
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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))),

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See my comment in #18799 (comment)

@geido
Copy link
Member

geido commented Mar 7, 2022

/testenv up

@github-actions
Copy link
Contributor

github-actions bot commented Mar 7, 2022

@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
Copy link
Member

geido commented Mar 7, 2022

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
Copy link
Member Author

ktmud commented Mar 7, 2022

@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

master-branch

my branch (this PR)

my-branch

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
Copy link
Member

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.

Copy link
Member

@michael-s-molina michael-s-molina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@geido geido left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@ktmud ktmud merged commit c75f233 into apache:master Mar 8, 2022
@github-actions
Copy link
Contributor

github-actions bot commented Mar 8, 2022

Ephemeral environment shutdown and build artifacts deleted.

@ktmud ktmud deleted the dataset-select-search-order branch March 8, 2022 17:01
villebro pushed a commit that referenced this pull request Apr 3, 2022
@mistercrunch 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🏷️ bot A label used by `supersetbot` to keep track of which PR where auto-tagged with release labels lts-v1 size/XL 🍒 1.5.0 🍒 1.5.1 🍒 1.5.2 🍒 1.5.3 🚢 2.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants