Skip to content
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

Add aria-activedescendant support in autocomplete #47

Merged
merged 4 commits into from
Oct 27, 2021

Conversation

yen-tt
Copy link
Contributor

@yen-tt yen-tt commented Oct 26, 2021

  • update searchbar input element's attributes to include aria-activedescedant based on selected autocomplete option using UP/DOWN key.

J=SLAP-1650
TEST=manual

see that search bar input contains aria-activedescendant with id of the selected autocomplete
see that search bar input remains in focus when moving through the autocomplete option
tab over components and see that autocomplete options is not registered individually
see that aria-activedescedant is removed when move back to input element + when hit enter/click submit

@coveralls
Copy link

coveralls commented Oct 26, 2021

Coverage Status

Coverage remained the same at 75.269% when pulling e95ebec on dev/aria-activedescendant into 198a852 on main.

<div
key={index}
className={className}
id={`${cssClasses.option}-${index}`}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure that we should use cssClasses.option here because those are css classes and not ids. With some of the new CSS injection work, the CSS class could be a tailwind class, and that wouldn't make sense to use here. Maybe instead we add an optionId prop which is a function which takes in the index as a number and returns a string which represents the id? Potentially the InputDropdown would have this prop as well

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah right. In the case, what do you think about an optionIdPrefix instead? so it would be id = optionIdPrefix-index. It's simpler and a function might not be necessary, but it would allow a bit more flexibility. I'm fine with either way so if you think a function is better I could make that change!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

An optionIdPrefix sounds good to me

@yen-tt yen-tt requested a review from cea2aj October 27, 2021 14:36
Copy link
Member

@cea2aj cea2aj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@yen-tt yen-tt merged commit 37050f2 into main Oct 27, 2021
@yen-tt yen-tt deleted the dev/aria-activedescendant branch October 27, 2021 15:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants