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-114 Update a11y docn for the numeric input components #5028

Merged
merged 6 commits into from
Oct 10, 2024

Conversation

ChrisLabattD2L
Copy link
Contributor

@ChrisLabattD2L ChrisLabattD2L commented Sep 30, 2024

Context for DAYL-114: Numeric Inputs > Improve a11y doc'n

The purpose of this PR is to update the documentation d2l-input-number and d2l-input-percent components to include more a11y-based information. Fortunately a bit of the work was already done for both of these components in this PR, when the labelled-mixin was changed to include the same a11y documentation that was needed here.

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

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.

Both input-number and input-percent have labelHidden which mentions aria-label. We likely want their description to be more along the lines of the date inputs ("Hides the label visually. Hidden labels are still read by screen readers so make sure to set an appropriate label.")

Other than the few comments LGTM!

components/inputs/input-number.js Outdated Show resolved Hide resolved
components/inputs/docs/input-numeric.md Outdated Show resolved Hide resolved
components/inputs/docs/input-numeric.md Outdated Show resolved Hide resolved
@ChrisLabattD2L
Copy link
Contributor Author

Both input-number and input-percent have labelHidden which mentions aria-label. We likely want their description to be more along the lines of the date inputs ("Hides the label visually. Hidden labels are still read by screen readers so make sure to set an appropriate label.")

Sounds good and makes sense! I'm wondering if labelHidden might be a good thing to move into the LabelledMixin, it appears in all the components that use it with the exception of d2l-selection-input. But that would be a change made outside the scope of this PR.

@margaree
Copy link
Contributor

margaree commented Oct 1, 2024

Definitely out of scope for this PR. It's an interesting thought. Basically the only reason labelHidden would be in LabelledMixin would be for having it defined in one place. However, it wouldn't really actually get used in there, since it's generally used for rendering and can be handled differently depending on the component, whereas LabelledMixin looks to be more used for finding/enforcing that a label is present but not actually handling how the label is rendered. Something to think about.

@ChrisLabattD2L ChrisLabattD2L marked this pull request as ready for review October 1, 2024 14:40
@ChrisLabattD2L ChrisLabattD2L requested a review from a team as a code owner October 1, 2024 14:40
Copy link
Contributor

@geurts geurts left a comment

Choose a reason for hiding this comment

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

Looks good to me Chris, thanks!

The first bullet in the Accessibility section made me want to know more about why it was setup that way, or how it affects the screen reader experience. Maybe we could add more context? Something like this:

When using unit make sure to also set unit-label since the unit itself is hidden from screen reader users; the unit-label is appended to the label off-screen so that it's announced by screen readers and hidden from sighted users.

@ChrisLabattD2L ChrisLabattD2L force-pushed the clabatt/DAYL-114-a11y-docn-numeric-inputs branch from 8452ac8 to a525258 Compare October 8, 2024 16:08
@ChrisLabattD2L ChrisLabattD2L merged commit 0f20030 into main Oct 10, 2024
6 checks passed
@ChrisLabattD2L ChrisLabattD2L deleted the clabatt/DAYL-114-a11y-docn-numeric-inputs branch October 10, 2024 14:15
Copy link

🎉 This PR is included in version 3.51.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