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

Adds ARIA attributes #232

Closed
wants to merge 1 commit into from

Conversation

nick-desteffen
Copy link
Contributor

@flavorjones
Copy link
Owner

Hi, @nick-desteffen! Thanks for submitting this. I just got back from a week on the road for work, so I'm catching up on open-source. I will take a look in the next day or two.

@flavorjones
Copy link
Owner

@nick-desteffen I'm curious why the following ARIA attributes aren't added in this PR:

                                  "aria-braillelabel",
                                  "aria-brailleroledescription",
                                  "aria-colindextext",
                                  "aria-rowindextext",

which are all listed on the Attributes page you referenced above. I've got a commit ready to push that adds these, but wanted to double-check with you that I'm not missing anything (I'm not very familiar with ARIA).

@nick-desteffen
Copy link
Contributor Author

@flavorjones I must have missed those, since they aren't listed under one of the 4 main categories.

They are all valid attributes according to the spec, so I think they should be included. Sorry about missing them. Their usage really depends on the screen reader and how compliant you want your application to be.

Official WC3 references:
https://w3c.github.io/aria/#aria-braillelabel
https://w3c.github.io/aria/#aria-brailleroledescription
https://w3c.github.io/aria/#aria-colindextext
https://w3c.github.io/aria/#aria-rowindextext

@flavorjones
Copy link
Owner

@nick-desteffen would you mind clicking the checkbox on the right that says "Allow edited by maintainers" so I can push an additional commit? I'd prefer that to opening a new PR.

image

Thanks!

@nick-desteffen
Copy link
Contributor Author

@flavorjones hmm, I'm not seeing that option. It could be because I didn't open a PR against our fork. I can just add those missing attributes and squash my commits.

@flavorjones
Copy link
Owner

Huh. OK, well I added a test, too, so I'm going to close this, open a second PR, but keep your commit in history and credit you in the CHANGELOG.

@flavorjones flavorjones mentioned this pull request Apr 28, 2022
@flavorjones
Copy link
Owner

Release has been made! Thanks for contibuting this!

https://github.com/flavorjones/loofah/releases/tag/v2.17.0

@nick-desteffen
Copy link
Contributor Author

🍻 Thank you!

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.

2 participants