-
Notifications
You must be signed in to change notification settings - Fork 204
fix: select, multiselect option grouping #4038
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
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #4038 +/- ##
=======================================
Coverage 96.99% 96.99%
=======================================
Files 861 861
Lines 25207 25207
Branches 9098 9097 -1
=======================================
Hits 24450 24450
- Misses 710 751 +41
+ Partials 47 6 -41 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| currentParent = wrapped; | ||
| nestedOptions.push(wrapped); | ||
| } else if (!option.type || option.type === 'child') { | ||
| } else if (option.type === 'child') { |
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.
What is the case for the option.type to be undefined?
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.
Assuming that root-level non-parent nodes have option.type set to undefined, I think this can be on the right direction, and simpler than my approach.
Could you add some testing coverage?
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.
What is the case for the
option.typeto be undefined?
@pan-kot
In the flatten-options.ts, the option.type = "child" will be set, if the option is inside the options array of another parent-option (e.g. groups). The parent-option will get the option.type = "parent" set.
@jperals Exactly. I will add some test cases.
5835cab to
ce9c3db
Compare
Description
This PR fixes a bug that groups "root"-level options in a select and multiselect dropdown to the previous group. The issue was non-visual, but leads to issues with accessibility screen readers.
Related Issues: [AWSUI-61453]
How has this been tested?
Review checklist
The following items are to be evaluated by the author(s) and the reviewer(s).
Correctness
CONTRIBUTING.md.CONTRIBUTING.md.Security
checkSafeUrlfunction.Testing
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.