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: [a11y docs] DAYL-93 Input Checkbox a11y docn #4977

Merged
merged 3 commits into from
Sep 24, 2024

Conversation

ChrisLabattD2L
Copy link
Contributor

@ChrisLabattD2L ChrisLabattD2L commented Sep 16, 2024

DAYL-93

This PR is updating the documentation for the d2l-input-checkbox component to denote the a11y aspects of the component. Adding the ACCESSIBILITY: prefix in from of a11y properties and a small Accessibility section at the end.

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-4977/

Note

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

components/inputs/docs/input-checkbox.md Show resolved Hide resolved
components/inputs/docs/input-checkbox.md Outdated Show resolved Hide resolved
components/inputs/docs/input-checkbox.md Outdated Show resolved Hide resolved
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.

LGTM. @geurts do you have any other thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

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

In addition to adding " for screen reader users" to the aria-label property, I'm wondering how we can clarify when to use aria-label vs the description property? There's no guidance on this, so we should either make it clear in the property descriptions or in a special note in the Accessibility section 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The more I think about it, the more I think what I wrote for aria-label is wrong, or at least the does not provide enough context part, since the providing more context is the responsibility of the description, not the label.

I'm thinking of changing it to: Use to override the text in the Default slot for screenreader users

What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not actually sure why we offer this. It might be helpful to find cases where it's used, to see if there are any clues as to why we introduced it?

Otherwise the "why" can just be left out, like you suggested. I would just start with "Overrides the text..." though for simplicity.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I seem to recall us having this type of discussion in one of our meetings, I think the end result was we'd leave it in for now, but would talk with Gaudi about it to see if it was something that could just get removed, or changed to just being label

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've updated it, but am going to leave this thread unresolved in case others wanted to weigh in on this topic

@ChrisLabattD2L ChrisLabattD2L marked this pull request as ready for review September 18, 2024 17:46
@ChrisLabattD2L ChrisLabattD2L requested a review from a team as a code owner September 18, 2024 17:46
@ChrisLabattD2L ChrisLabattD2L merged commit 533baf9 into main Sep 24, 2024
6 checks passed
@ChrisLabattD2L ChrisLabattD2L deleted the clabatt/a11y-docn-checkbox branch September 24, 2024 14:38
Copy link

🎉 This PR is included in version 3.44.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants