-
Notifications
You must be signed in to change notification settings - Fork 332
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
feat(core): introduce new completion system #354
Conversation
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit d224bb0:
|
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.
much better without the other div to align!
@@ -180,14 +180,14 @@ export function getPropGetters<TItem, TEvent, TMouseEvent, TKeyboardEvent>({ | |||
const { inputElement, maxLength = 512, ...rest } = providedProps || {}; | |||
|
|||
return { | |||
'aria-autocomplete': props.enableCompletion ? 'both' : 'list', | |||
'aria-autocomplete': 'both', |
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.
For a textbox with the aria-autocomplete attribute set to either inline or both, authors should ensure that any auto-completed text is selected, so the user can type over it.
https://accessibilityresources.org/aria-autocomplete
I don't see any sites doing this though, so I don't think it really makes sense like that
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.
From https://www.digitala11y.com/aria-autocomplete-properties/:
It distinguishes between two models: the inline model (aria-autocomplete=”inline”) that presents a value completion prediction inside the text input and the list model (aria-autocomplete=”list”) that presents a collection of possible values in a separate element that pops up adjacent to the text input. It is possible for an input to offer both models at the same time (aria-autocomplete=”both”).
Since we provide both the completion in the input and suggestions in the dropdown, I believe both
is the right value for us.
When I press ESC, the completion remains.
But I think ESC has the meaning of cancellation, we might want to roll back to the query, not the completion. Look at the example from Google: |
@eunjae-lee Yeah I was thinking about this last night. Esc and outside click have different meanings. I implemented that in d224bb0. Lmk! |
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.
nice 👍
This introduces a brand new completion experience baked-in Autocomplete by default. This reproduces exactly (according to my UX tests) experiences as seen on Google or Amazon.
Details
This new completion system replaces the input value when selected an item via keyboard (not mouse over), but doesn't set the query. The query is only set when re-interacting with the input, or opening the suggestions again (to not end up in a broken state where your input is the query and your suggestions are based on the completion on a new dropdown opening).
We'll reintroduce the previous feature if needed after stable release, rebranded "prediction". It will be more powerful by providing a query hint coming from an agnostic source (Autocomplete source, custom backend, AI server, etc.) that you can "validate" by hitting Tab. This is not what the feature in this PR is about.
The concept of "state enhancers" is removed from the codebase because there's no need for them with how this feature works now.
Our UI libraries (e.g., Autocomplete JS) don't even need to implement anything because it's all handled in the core.
Preview
Notes: