Add option groups to Select component #431
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* Adds more tests for the existing behaviors of Select, plus tests for OptionMenuList - this is to facilitate future changes to Select. * Fixes a bug in Select where a disabled option could still be selected via the keyboard.
caseywilliams
force-pushed
the
select-option-groups
branch
from
3c21d84 to
5668742
Compare
eputnam
approved these changes
May 27, 2021
| } from '../../source/react/constants'; | ||
|
|
||
| // Example with option groups | ||
| const options = [ |
There was a problem hiding this comment.
nittiest of nits: call this optionsWithGroups?
| return eventObj; | ||
| }; | ||
|
|
||
| const MANY_TIMES = 8; // (more times than the number of focusable items in these examples) |
There was a problem hiding this comment.
this made me laugh, i'm not sure why.
* OptionMenuListItem has a new prop, `type`, which can be set to either `option` (the default) or `heading` (an option group heading). The usual OptionMenuListItem event handlers and icons are ignored for option group headings. * OptionMenuList now accepts arrays of child options as option values; these values will be rendered as an option group. The parent item's label will be used as the option group heading. If the parent item is disabled, all children will also be disabled.
caseywilliams
force-pushed
the
select-option-groups
branch
from
5668742 to
ff1b7ae
Compare
scotje
approved these changes
May 27, 2021
Adds a small border between groups to help the user distinguish between them.
|
|
|
Made a tiny tweak to add in some borders. LGTM! |
|
Thanks @Sigler I'll fix up these linting errors and add a changelog entry before I merge! |
Also fixes a linting error
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR implements option groups for the Select component (and by extension, the OptionMenuList component) - I've been working off of this design for the forge.
In short, this makes it so that for Select components, if you specify an array of options as an option value, the array items will be the child options, and the parent's label will be the option group title. I have not enabled focus or click/select events for group titles (but happy to do so if wanted).
This was a pretty large change, but I added several new tests for OptionMenuList and Select to try to ensure all existing behavior stayed the same. This also fixes a small bug in Select where disabled items could be selected via keyboard.