fix: add arialabel for pagination and select elements #20043
Conversation
|
The default texts mentioned for the ariaLabel input is referred from carbon-components-angular repo. Not sure if we support key translations here |
✅ Deploy Preview for v11-carbon-web-components ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for carbon-elements ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for v11-carbon-react ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
Codecov Report✅ All modified and coverable lines are covered by tests.
Additional details and impacted files@@ Coverage Diff @@
## main #20043 +/- ##
==========================================
- Coverage 91.39% 85.03% -6.36%
==========================================
Files 485 355 -130
Lines 31370 14393 -16977
Branches 5430 4819 -611
==========================================
- Hits 28670 12239 -16431
+ Misses 2547 2014 -533
+ Partials 153 140 -13
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| ?disabled="${prevButtonDisabled}" | ||
| button-class-name="${prevButtonClasses}" | ||
| tooltip-text="${backwardText}" | ||
| aria-label="Backward" |
There was a problem hiding this comment.
Why not use the backwardText?
There was a problem hiding this comment.
Both the review comments are addressed
| ?disabled="${nextButtonDisabled}" | ||
| button-class-name="${nextButtonClasses}" | ||
| tooltip-text="${forwardText}" | ||
| aria-label="Forward" |
There was a problem hiding this comment.
Same here, why not forwardText?
|
@r1shabhsharma if you need any help, just let us know! |
|
@heloiselui Sure, Thanks! |
|
Can we merge this PR? Or get some reviews please? |
| * Specify text for the accessibility label of the button | ||
| */ | ||
| @property({ attribute: 'aria-label' }) | ||
| ariaLabel = ''; |
There was a problem hiding this comment.
This should be undefined by default I think, rather than an empty string. This is why it's being included in the snapshot when I don't think it should be.
| ariaLabel = ''; | |
| ariaLabel!: string; |
| ?disabled="${disabled}" | ||
| aria-readonly="${String(Boolean(readonly))}" | ||
| aria-invalid="${String(Boolean(invalid))}" | ||
| aria-label=${ifDefined(ariaLabel)} |
There was a problem hiding this comment.
I think the way to fix this isn't through an aria-label, but refactoring the hideLabel logic down on L498.
In the react version, Select always has a label. hideLabel toggles a class to visibly hide the label, not completely omit it from the dom like the web components one does here. I think this would fix most/all situations where you'd reach for an aria-label and would make it more accessible out of the box.
|
Hey, we’re closing this PR in favor of this one: #21051. Thanks a lot for your contribution! |
4 participants
Closes #20042
This PR includes:
Before:
After:
Changelog
New
Changed
Removed
Testing / Reviewing
{{ Add steps or a checklist for how reviewers can verify this PR works or not }}
PR Checklist
As the author of this PR, before marking ready for review, confirm you:
More details can be found in the pull request guide