USWDS - Combo box: Enhance combo box results #6122
Conversation
| if (disableFiltering || isPristine) { | ||
| options.push(option); |
There was a problem hiding this comment.
note: adding isPristine here prevents selected values from appearing at the top of the combo box. This allows the selected item to appear in the correct order, which matches current combo box behavior
There was a problem hiding this comment.
This is looking really good. I am still doing some edge case testing, but so far the results are returning as expected. A nice improvement!
In the meantime, I wanted to share some suggestions and questions to improve code readability. I'll cycle back with the results of the other review checks.
There was a problem hiding this comment.
Great work here @mahoneycm . Feels well-organized and well-scoped. Excited about this improvement.
Just have a few open comments about improving comments and naming for readability.
Testing
I completed the following checks using these queries:
Combo box: “Z”, “Ca”, “tan”, “go”, “orange”
Time picker: “1”, “12”, “12pm”
- Confirm that combo box filters out options that do not match the query (pre-existing feature)
- Confirm that time picker does not filter out options that do not match the query (pre-existing feature)
- Confirm that the combo box sorts the filtered options by first showing content that “starts with” the query, and follows with results that "contain" the query
- Confirm that combo box filters and sorts by the option text content, not the option
value- Tested by adding a
valueof “hamburger” to an option with text content of "Apple". Querying “hamburger” did not return any results. - Confirm that this approach makes sense
- Tested by adding a
- Confirm that deleting a character from a selected values re-triggers the filter and sort
- Confirm that upper- or lower-casing in the text content does not change the expected search results
- Confirm code meets uswds standards
- Confirm code has needed JSDoc comments
- Confirm unit tests make sense and pass
There was a problem hiding this comment.
Overall the component functions much better, nice job!
Made comments to improve code quality and readability.
Additionally, I found a potential performance issue that we can split into another issue. Mainly because it's already happening in develop and level of effort might be beyond the scope of this issue.
| assert.ok(!list.hidden, "should display the option list"); | ||
| assert.strictEqual( | ||
| list.children.length, | ||
| 3, | ||
| "should filter the item by the string being present in the option", | ||
| ); |
There was a problem hiding this comment.
polish: The message could be misinterpreted.
| assert.ok(!list.hidden, "should display the option list"); | |
| assert.strictEqual( | |
| list.children.length, | |
| 3, | |
| "should filter the item by the string being present in the option", | |
| ); | |
| assert.ok(!list.hidden, "should display a filtered list based on input"); | |
| assert.strictEqual( | |
| list.children.length, | |
| 3 | |
| ); |
There was a problem hiding this comment.
This was the test string used in each of the other filter tests so I reused for consistency. I updated this test description in e292603 but did not update the other test descriptions to reduce changed lines.
assert.strictEqual(
list.children.length,
3,
"should display a filtered list based on input",
);Let me know if you want me to go ahead update the other testing descriptions to follow the new pattern
There was a problem hiding this comment.
praise: Thanks for updating and adding unit tests!
Update from dev syncUsing
|
There was a problem hiding this comment.
Looks good to me! Both combo box and time picker are behaving as intended. All blocking comments have been addressed.
Overall, this feels like a nice, well-scoped change with some great readability improvements. Looking forward to seeing further improvements in #6193.
I've added a few comments with suggestions for improving the code comments, but don't think they need to block anything.
I've documented my tests in the section below:
Tests performed
- Confirm that combo box filters out options that do not match the query (pre-existing feature)
- Confirm that time picker does not filter out options that do not match the query (pre-existing feature)
- Confirm that the combo box sorts the filtered options by first showing content that “starts with” the query, and follows with results that "contain" the query
- Confirm the first item in the display list receives focus
- Confirm that combo box filters and sorts by the option text content, not the option
value - Confirm that deleting a character from a selected values re-triggers the filter and sort
- Confirm that upper- or lower-casing in the text content does not change the expected search results
- Confirm code is readable and organized
- Confirm code has needed JSDoc comments
- Confirm unit tests make sense and pass
Co-authored-by: Amy Leadem <93996430+amyleadem@users.noreply.github.com>
Summary
Updated combo box result filtering. Now, search results are organized by results that start with the query, followed by results that contain the query.
Breaking change
This is not a breaking change.
Related issue
Closes #5647. Closes #5522.
Related pull requests
Change log PR →
Preview link
Combo box →
Time picker →
Problem statement
Combo box search results do not match user expectations. Keyboard and mouse users expect to see results that closely match their query.
Solution
By pushing results that start with their query closer to their top, we can more closely match user expectations.
Major changes
Testing and review
tanTesting checklist
Additional information
I found testing with additonal select options beneficial during my work. Here's some example markup you can replace in
usa-combo-box.twigto try the new behaviors with different results.State select markup