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.
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
Components: Add a WAI-ARIA compliant Combobox. #19657
Components: Add a WAI-ARIA compliant Combobox. #19657
Changes from all commits
6be6030
ac050ac
20b2cbf
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
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.
no change here right?
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.
Nope
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 looks like it should be a downshift bug.
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.
@epiqueras can you point out to me the use case when this happens? I want to fix it in v5. Thanks!
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.
When mounting in uncontrolled mode.
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 am confused, sorry :)). Do we have an example somewhere?
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.
https://github.com/downshift-js/downshift/tree/master/src/hooks/useSelect#usage
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.
@epiqueras downshift-js/downshift#945 is this fixing your use case?
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.
It should, yeah.
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.
Can we remove this code if it's fixed in Downshift?
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.
Again, if we're following Example 1, I don't think we need a button here at all.
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 think it's valuable to have a button on the right to open the list without typing, isn't it?
It's what Downshift recommends too.
cc @silviuavram
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 going by the WAI-ARIA patterns linked to above, where the only example with a button is the inline autocomplete one. That's not the one we're implementing here, so thought it would be good to stay consistent with their recommendations.
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.
Yes, there are versions with the toggle and versions without. The toggle is helpful to see the options before typing anything, but if that is not part of the requirement then it can be ommited. :)
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.
Should we keep it? It looks quite nice.
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.
Let's keep it for now, shouldn't do any harm 🙂
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.
Can we check that the input has a non-empty value before rendering the items? I'm still seeing the bug where, after typing in a letter and then deleting it so the input becomes empty, the whole list shows. Or perhaps this should be fixed in Downshift instead? Cc @silviuaavram
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.
isOpen
does not take into account the value ininputValue
, so you can have the menu open without having anything in the input. Is there an ARIA/general agreed behavior for this? I am not aware of it.To customise you can probably use
stateReducer
and only makeisOpen
true ifinputValue
has content. If you consider the default behavior as a bug, can you open an issue in Downshift and have it documented? Our hooks are very easy to customise behavior, but we should have the standard actions on par with ARIA guidelines.Thanks!
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.
Yup, that's the behaviour in the official ARIA combobox examples. Check the third example, that has the button just like ours. The whole menu is only visible on click of the button, or on Arrow Down keypress. But if you type in the box and then clear, the dropdown disappears.