-
Notifications
You must be signed in to change notification settings - Fork 13.9k
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
Conversation
6ef5aed
to
4edf089
Compare
8de112c
to
8cb01af
Compare
Codecov Report
@@ 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
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
aa93ba6
to
b216b3b
Compare
@michael-s-molina @geido this is ready for review. cc @etr2460 since you've also worked on this area before. |
c3a0a4d
to
144d79a
Compare
const allowFetch = !fetchOnlyOnSearch || searchedValue; | ||
|
||
// TODO: Don't assume that isAsync is always labelInValue | ||
const handleTopOptions = useCallback( |
There was a problem hiding this comment.
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(); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
1a1881f
to
d01a32e
Compare
There was a problem hiding this 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
@@ -577,7 +538,6 @@ const Select = ({ | |||
|
|||
if (filterOption) { | |||
const searchValue = search.trim().toLowerCase(); |
There was a problem hiding this comment.
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.
9c8a546
to
eea20ee
Compare
48d1668
to
e706228
Compare
@@ -153,11 +153,12 @@ describe('SelectControl', () => { | |||
}); | |||
|
|||
describe('when select has a sortComparator prop', () => { | |||
it('does not add add order key and sorts by sortComparator', () => { |
There was a problem hiding this comment.
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( |
There was a problem hiding this comment.
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".
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
optionsArray.find( | ||
x => | ||
x === value || | ||
(typeof x === 'object' && | ||
(('value' in x && x.value === value) || | ||
(checkLabel && 'label' in x && x.label === value))), |
There was a problem hiding this comment.
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
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))), |
There was a problem hiding this comment.
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)
/testenv up |
@geido Ephemeral environment spinning up at http://54.185.166.79:8080. Credentials are |
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! |
@michael-s-molina I can't reproduce what you described in both master branchmy 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. |
@ktmud @geido The problem is not related to the PR changes. We'll handle it in another PR. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
Ephemeral environment shutdown and build artifacts deleted. |
(cherry picked from commit c75f233)
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