fix(switcher): wrap SwitcherDivider hr in li for a11y compliance#20632
Conversation
|
All contributors have signed the DCO. |
✅ 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!
To edit notification comments on pull requests, go to your Netlify project configuration. |
emyarod
left a comment
There was a problem hiding this comment.
I currently see this duplicated horizontal rule, and I believe the class names need to be placed on the hr to resolve this
d31e653 to
d3fda7f
Compare
|
@emyarod |
|
@naaa760 You'll need to complete the DCO before this can be merged. Instructions in the comment above #20632 (comment) |
tay1orjones
left a comment
There was a problem hiding this comment.
Duplicate is still present
| <SwitcherItem aria-label="Link 1" href="#"> | ||
| Link 1 | ||
| </SwitcherItem> | ||
| <SwitcherDivider /> |
There was a problem hiding this comment.
As @emyarod stated, the styles need to be modified to apply to the correct element. Removing the SwitcherDivider from the story is not a fix for the problem.
|
I have read the DCO document and I hereby sign the DCO. |
e2c7496 to
0fa6555
Compare
|
@tay1orjones |
tay1orjones
left a comment
There was a problem hiding this comment.
Does this work? I think the classnames should apply to the same element as before.
| <li className={classNames}> | ||
| <hr {...other} /> | ||
| </li> |
There was a problem hiding this comment.
| <li className={classNames}> | |
| <hr {...other} /> | |
| </li> | |
| <li> | |
| <hr {...other} className={classNames} /> | |
| </li> |
|
|
||
| hr { | ||
| border: none; | ||
| margin: 0; | ||
| padding: 0; | ||
| background: transparent; | ||
| block-size: 1px; | ||
| inline-size: 100%; | ||
| } |
There was a problem hiding this comment.
| hr { | |
| border: none; | |
| margin: 0; | |
| padding: 0; | |
| background: transparent; | |
| block-size: 1px; | |
| inline-size: 100%; | |
| } |
7876815 to
d2e7328
Compare
|
@tay1orjones |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #20632 +/- ##
==========================================
- Coverage 95.15% 95.13% -0.02%
==========================================
Files 549 549
Lines 45650 45650
Branches 6532 6575 +43
==========================================
- Hits 43437 43429 -8
- Misses 2083 2091 +8
Partials 130 130
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:
|
|
@naaa760 |
Co-authored-by: Mahmoud <132728978+maradwan26@users.noreply.github.com>
tay1orjones
left a comment
There was a problem hiding this comment.
I've updated this and added tests. LGTM
9cdf860
closes #20619
<hr>element directly inside a<ul>, which violates WCAG 2.1 standards. This fix wraps the<hr>in an<li>element to maintain proper list structure while preserving the visual appearance and functionality.Changed
<hr>wrapped in<li>instead of standalone<hr>for accessibility complianceTesting / Reviewing