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

feat: DAYL-96 Update Count Badge a11y docn #5077

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

ChrisLabattD2L
Copy link
Contributor

@ChrisLabattD2L ChrisLabattD2L commented Oct 16, 2024

Context for DAYL-96: Count Badge > Improve a11y doc'n

The purpose of this PR is to update the a11y documentation for the d2l-count-badge and d2l-count-badge-icon components. This primarily includes adding the ACCESSIBILITY: prefix in front of accessibility-based components and adding a section at the end of the readme with some a11y considerations.

I also had to update the properties tables in the readme, due to them being formatted differently than other readmes, so I've made them more consistent (the default and required fields are with the type now)

Copy link
Contributor

Thanks for the PR! 🎉

We've deployed an automatic preview for this PR - you can see your changes here:

URL https://live.d2l.dev/prs/BrightspaceUI/core/pr-5077/

Note

The build needs to finish before your changes are deployed.
Changes to the PR will automatically update the instance.

Copy link
Contributor

@margaree margaree left a comment

Choose a reason for hiding this comment

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

Thanks for fixing up the README properties tables too!

components/count-badge/count-badge-mixin.js Outdated Show resolved Hide resolved
components/count-badge/count-badge-mixin.js Outdated Show resolved Hide resolved
components/count-badge/count-badge-mixin.js Outdated Show resolved Hide resolved
components/count-badge/count-badge-mixin.js Outdated Show resolved Hide resolved
Comment on lines +76 to +80
- While non-interactable components traditionally shouldn't be focusable, the `tab-stop` property greatly helps screen-reader users find the the count badge
- Otherwise, screen-reader users would have to use the arrow keys to get to the badge, which can take considerably longer
- If the optional tooltip is used, then `tab-stop` isn't necessary, as the count-badge becomes focusable to render the tooltip
- The optional tooltip makes use of the [`d2l-toolip`](../../components/tooltip), which follows the [W3C best practices for Tooltips](https://www.w3.org/WAI/ARIA/apg/patterns/tooltip/)
- The use of this tooltip can help provide additional context such as providing the exact number, if it were to go above the limit set in `max-digits`
Copy link
Contributor

Choose a reason for hiding this comment

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

Just trying to make it a bit more clear, though probably needs @geurts input on wording

Suggested change
- While non-interactable components traditionally shouldn't be focusable, the `tab-stop` property greatly helps screen-reader users find the the count badge
- Otherwise, screen-reader users would have to use the arrow keys to get to the badge, which can take considerably longer
- If the optional tooltip is used, then `tab-stop` isn't necessary, as the count-badge becomes focusable to render the tooltip
- The optional tooltip makes use of the [`d2l-toolip`](../../components/tooltip), which follows the [W3C best practices for Tooltips](https://www.w3.org/WAI/ARIA/apg/patterns/tooltip/)
- The use of this tooltip can help provide additional context such as providing the exact number, if it were to go above the limit set in `max-digits`
Notable accessibility features:
- `tab-stop`:
- While non-interactable components traditionally shouldn't be focusable, this property greatly helps screen reader users find the the count badge, and removes the need to use the arrow keys to navigate to it.
- If the optional tooltip is used, it isn't necessary, as the count-badge becomes focusable in order to display the tooltip
- `has-tooltip`:
- The optional tooltip makes use of the [`d2l-tooltip`](../../components/tooltip), which follows the [W3C best practices for Tooltips](https://www.w3.org/WAI/ARIA/apg/patterns/tooltip/)
- This tooltip can help provide additional context, such as by providing the exact count badge number if it were to go above the limit set in `max-digits`

Copy link
Contributor Author

Choose a reason for hiding this comment

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

While I do like some of the changes, I'm not sure about the formatting, so I'd also likely to hear Jeff's input

Copy link
Contributor

Choose a reason for hiding this comment

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

Totally fair!

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