-
Notifications
You must be signed in to change notification settings - Fork 144
Conversation
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.
@joshuatf This works great. The only thing I found is that it needs docs/packages/components/autocomplete.md
for the single component doc page to work.
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 testing well for me. All CLI tests are passing as well. Besides my minor comment, we should add the documentation Ron noted below too.
The label/placeholder hiding is a bit different from existing newspack components (I believe they were incorrect and this component more closely mirrors those seen in Material).
If I recall correctly, I had to adjust the behavior in the calypso-ui
port of TextControl
as well. I think this is another reason to standardize/move components somewhere we can have more control over.
This display of this with inline tags looks a bit off to me:
I think the label should be above the tags and text, like it would be in a text input.
Should this component offer an option to hide the search field, allowing it to become a styled select component? This may consolidate efforts between various components. If "yes," should this be renamed to Select or something similar?
That would be cool, and would help reduce the different components we have floating around (where a prop might suffice). We'll probably want to log an issue for the consolidation effort.
0b53492
to
c184468
Compare
Thanks for the review @rrennick and @justinshreve 😄
Added these and other missing doc files.
I think you're right. One small side effect from doing this means tags need to be reduced in height slightly: Also applied other feedback and gave this PR a rebase. |
@joshuatf Looks great for me. |
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.
Changes look good and are testing well 👍.
c184468
to
3229ccd
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.
Amazing job with this @joshuatf! Sorry for the late review. I think this is a great start and gets us what we need. I found a few things off with keyboard interactions and noted two that I think are important because keyboard only users can get stuck.
Another really nice addition would be a focus state on the inputs.
const filtered = []; | ||
|
||
// Create a regular expression to filter the options. | ||
const expression = getSearchExpression( escapeRegExp( query.trim() ) ); |
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.
On inline tags with the options always visible, pressing the escape key results in an error index.js:126 Uncaught TypeError: Cannot read property 'trim' of null
.
Tab should also allow me to move on, but using only the keyboard you can get stuck. See the other comment.
event.stopPropagation(); | ||
break; | ||
|
||
case TAB: |
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.
The tab functionality in https://www.w3.org/TR/wai-aria-practices/examples/combobox/aria1.1pattern/listbox-combo.html brings focus to the next focusable element even when the options are visible.
Here, tab brings me to the next focusable element only when the options are hidden. In the case of the first example in dev docs, if you navigate to it using tab, you can't tab out of it unless something is selected.
Fixes #2527
Adds an autocomplete component with filterable options.
There are a few areas this does not cover and will be handled in a follow-up since this PR is already rather large:
Autocomplete
component inside theSearch
component.Questions
Select
or something similar?Accessibility
Screenshots
Detailed test instructions:
Here are some references to check against in terms of accessibility standards (note this uses ARIA 1.0 as it seems this still has more support than 1.1 and there aren't plans to deprecate):