Skip to content

Add aria-multiselectable #1726

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.

Sign up for GitHub

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

Jump to bottom
Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add aria-multiselectable #1726

wants to merge 1 commit into from

Conversation

@Pytal
Copy link
Contributor

@Pytal Pytal commented Dec 14, 2022

Enhance component accessibiltiy as described in the MDN docs:

@netlify
Copy link

netlify bot commented Dec 14, 2022 •
edited
Loading

👷 Deploy request for vueselect accepted.

Name Link
🔨 Latest commit 3d17871
🔍 Latest deploy log https://app.netlify.com/sites/vueselect/deploys/639bd748171e77000b6210fb

@Pytal Pytal mentioned this pull request Dec 14, 2022
Closed
11 tasks
@sagalbot
Copy link
Owner

sagalbot commented Dec 15, 2022

If you could add a test for the attribute value here we can get this one released. Thanks for the efforts!

@Pytal Pytal force-pushed the enh/a11y-aria-multiselectable branch from e082b2c to 3d17871 Compare December 16, 2022 02:26
@Pytal
Copy link
Contributor Author

Pytal commented Dec 16, 2022

Tests added!

sagalbot
sagalbot requested changes Dec 16, 2022
View reviewed changes
src/components/Select.vue Outdated
if (this.multiple) {
return this.isOptionSelected(option)
}
return index === this.typeAheadPointer ? true : null
Copy link
Owner

@sagalbot sagalbot Dec 16, 2022

Choose a reason for hiding this comment

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

I think my original functionality here was actually incorrect. I was using aria-selected to denote that the option is currently highlighted, but after rereading the docs, it should be used to denote the current selection. I think it should probably be this.isOptionSelected in all cases.

Let me know if you think I'm incorrect there.

Copy link
Contributor Author

@Pytal Pytal Jan 4, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Agreed and updated!

Also converted the boolean to string to match the docs https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Attributes/aria-selected#values (otherwise false would omit the attr and make it seem unselectable) with updated tests!

Copy link
Contributor Author

@Pytal Pytal Jan 17, 2023

Choose a reason for hiding this comment

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

Look good now @sagalbot?

@Pytal Pytal force-pushed the enh/a11y-aria-multiselectable branch 2 times, most recently from b5f38fe to 882e42d Compare January 4, 2023 02:29
@Pytal Pytal force-pushed the enh/a11y-aria-multiselectable branch from 882e42d to 5e1fad0 Compare January 5, 2023 02:40
@AndyScherzinger
Copy link

AndyScherzinger commented Jan 26, 2023

Hi @sagalbot ,
first of all let me say thank you for all the work you are doing in offering this component, this is highly appreciated and we love your work. Not wanting to put any pressure on you just wanting to ask to have a clear view on a timeline-dimension, do you have a gut-feeling if or when you might be able to take another look at this PR and @Pytal's changes based on your latest review? Same goes for #1742. And again no pressure, it is a privilege to be able to use this component you are building and maintaining ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@sagalbot sagalbot sagalbot requested changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@Pytal @sagalbot @AndyScherzinger