-
Notifications
You must be signed in to change notification settings - Fork 2
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
Create SelectNext component replacing Select #1247
Conversation
3c167ac
to
69be4d7
Compare
deb4942
to
c51250f
Compare
Codecov Report
@@ Coverage Diff @@
## main #1247 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 59 61 +2
Lines 790 836 +46
Branches 318 327 +9
=========================================
+ Hits 790 836 +46
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
c51250f
to
65d6ae7
Compare
Some accessibility things I noticed in testing with VoiceOver:
I think the general pattern that we want to follow here is the Select-Only combobox. In addition to the two items mentioned above, there are other details of keyboard handling etc. in that document. In my testing, setting This PR is quite large now so I suggest we resolve that, and then create a follow-up issue for the remaining items. |
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 is generally looking in good shape. There are a few more things to address around keyboard input and accessibility. If we're agreed on following the "select-only combobox" pattern for this, then I think setting role="combobox"
is enough to get this to a state where we can merge this version, and then a follow-up issue can be opened for the remaining items.
src/pattern-library/components/patterns/prototype/SelectNextPage.tsx
Outdated
Show resolved
Hide resolved
src/pattern-library/components/patterns/prototype/SelectNextPage.tsx
Outdated
Show resolved
Hide resolved
src/pattern-library/components/patterns/prototype/SelectNextPage.tsx
Outdated
Show resolved
Hide resolved
src/pattern-library/components/patterns/prototype/SelectNextPage.tsx
Outdated
Show resolved
Hide resolved
65d6ae7
to
9d243dc
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.
Listbox popup button is now announced as expected, including collapsed / expanded state.
9d243dc
to
c0b16c7
Compare
Create a new
SelectNext
component, which is meant to replace existingSelect
one.The benefits it presents include being able to customize the listbox UI, and supporting complex objects as values.
SelectNext
is a controlled component designed to be used like this:Existing
Select
is now marked as deprecated, and the plan is to remove it on next major release, while we also renameSelectNext
to plainSelect
.TODO
Follow up improvements
There are a couple of things we can improve, but it would be better to tackle separately to avoid a PR bigger than it already is:
Nice to have:
feedback="error"
andfeedback="warning"
like the existing Select does.Closes #1223