feat(header-menu-item): add isActive prop to apply styles to selected item #5504
Conversation
abbeyhrt
requested review from
a team,
jeanservaas and
laurenmrice
and removed request for
a team and
jeanservaas
ghost
removed their request for review
|
Deploy preview for carbon-elements ready! Built with commit ca96865 |
|
Deploy preview for carbon-components-react ready! Built with commit ca96865 https://deploy-preview-5504--carbon-components-react.netlify.com |
|
Deploy preview for carbon-elements ready! Built with commit 5fbfece |
|
Deploy preview for carbon-components-react failed. Built with commit b8dc12e https://app.netlify.com/sites/carbon-components-react/deploys/5e8bc2e90545bf0006ae63a5 |
|
Deploy preview for carbon-elements ready! Built with commit b8dc12e |
| ref | ||
| ) { | ||
| return ( | ||
| <li className={className} role={role}> | ||
| <Link | ||
| {...rest} | ||
| aria-current={isActive ? 'page' : null} |
There was a problem hiding this comment.
Would we just want them to pass in <HeaderMenuItem aria-current="page" /> as a prop? Similarly, would we want to use isActive to specify a custom selector if they don't want to use the ARIA tech? (Similar to what we do over in breadcrumbs)
There was a problem hiding this comment.
Just sharing an experience; Using ARIA attribute for a styling purpose has caused a breaking change when we made an a11y structure fix/change.
There was a problem hiding this comment.
I changed it to match the pattern we use in Breadcrumb. Thanks for the insight @joshblack and @asudoh! Definitely think not wholly on the ARIA attributes is a good way to go here 👍
…rbon-fork into fix-3674-ui-shell
|
@abbeyhrt small visual bug I'm noticing
Might just need a transparent border on the inactive links? Otherwise, the changes look great! |
|
@abbeyhrt we can try something like a.#{$prefix}--header__menu-item[aria-current='page']:focus,
a.#{$prefix}--header__menu-item.#{$prefix}--header__menu-item--current:focus {
border-bottom: 2px solid $shell-header-focus;
outline: 2px solid $shell-header-focus;
} |
| // We set the current class only if `isCurrentPage` is passed in and we do | ||
| // not have an `aria-current="page"` set for the breadcrumb item | ||
| [`${prefix}--header__menu-item--current`]: | ||
| isCurrentPage && ariaCurrent !== 'page', |
There was a problem hiding this comment.
Usage of ARIA attribute content for styling has caused breaking changes during our a11y fix effort. So wondering if just isCurrentPage is enough.
There was a problem hiding this comment.
I don't think we'll run into any problems since it's being implemented in both ways. It definitely is risky to style using ARIA roles since we might change those based on new info but aria-current="page" is more stable and not likely to change. I also think it's nice to support both use cases for the ways that people might try to implement current page styling.
There was a problem hiding this comment.
aria-current="page"is more stable and not likely to change
Probably it may change (esp. in post-IE era) because of newer spec in this area is on the horizon https://wicg.github.io/aom/explainer.html
There was a problem hiding this comment.
It seems like it'd be good to go for now, given the class fallback. If there is a specific line or section from the spec that you think is relevant and should be included in the PR that'd be great to highlight 👍 aria-current with page seems like a fairly consistent attribute for communicating present navigation to screen readers.
|
@laurenmrice my bad! I didn't actually push the new changes 🤦♀ |
| // We set the current class only if `isCurrentPage` is passed in and we do | ||
| // not have an `aria-current="page"` set for the breadcrumb item | ||
| [`${prefix}--header__menu-item--current`]: | ||
| isCurrentPage && ariaCurrent !== 'page', |
There was a problem hiding this comment.
Just in case if this clarifies what the code should be:
| isCurrentPage && ariaCurrent !== 'page', | |
| isCurrentPage, |
Closes #3674
This PR adds an
isActiveprop that determines whichHeaderMenuItemshould havearia-current="page"and its corresponding styles applied. It also edits one of the UIShell's stories to show an example of the new style. This removes theborder-bottomon the Header bar since it didn't seems to be there in the design specs and it was causing a black line underneath the focus and the selected states.Changelog
New
isActivepropHeaderMenuItemRemoved
border-bottomfrom the HeaderTesting / Reviewing
Designers: check that the new styles are how we want to selected state to look, you can see an example of the selected state in UIShell's
Header Base w/ Navigationstory.Dev: Is this how we want aria-current to be applied?
BreadcrumbItemdoes the same thing in a different way and I'm not sure what is the best way to go about this.