-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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(Tabs): replace mobile dropdown tabs with overflow nav buttons #6410
Conversation
the storybook preview will depend on #6403 |
Deploy preview for carbon-elements ready! Built with commit 8de39bd |
Deploy preview for carbon-components-react ready! Built with commit 8de39bd https://deploy-preview-6410--carbon-components-react.netlify.app |
Deploy preview for carbon-elements ready! Built with commit 44d55cb |
Deploy preview for carbon-components-react ready! Built without sensitive environment variables with commit 44d55cb https://deploy-preview-6410--carbon-components-react.netlify.app |
@aagonzales should be fixed once the storybook is re-deployed, fix was just merged in 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's looking good! Just a couple of bugs.
- The disabled border is disappearing in the light variants. Using
disabled-02
won't work because it will actually make it higher contrast than the other unselected line. Let's update the disabled border-bottom to keep usingui-03
like the unselected tabs. - Container tabs label spacing is off. The label should be vertically centered in the tab.
- Is there anything we can do about the scroll bar showing up when using the keyboard and when you click the arrow?
544de88
to
1264bc5
Compare
locally I couldn't reproduce the container tab label issue but I think I resolved it in storybook. the light disabled tabs should now have the updated color token. as for the scroll bar issue, I'm not sure what we can do about that especially since it also changes depending on the user's operating system etc. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! 🎉
the mouse scrollbar issue is probably due to the component width changing when the overflow button is rendered, not sure why it is only present for mouse but I may have to check somehow if the user is scrolling with scroll wheel / track pad versus dragging the scroll bar the auto scroll on focus is only implemented for the first and last tabs currently but I can look into recalculating scroll position for every navigation action |
<div | ||
{...other} | ||
className={classes.tabs} | ||
role={role} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need this role="nav"
? Having the tabs in a tablist
should be enough as far as roles are concerned unless there's something I'm missing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is carried over from the existing implementation (carbon-design-system/carbon-components-react#2318). what is the recommendation here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just took a look at our design guidance for tabs
If the tabs are 'default' then the nav role makes sense actually. If it's a 'container' version then we can follow the APG guidance and leave it out. If that's a pain in the neck having the nav role in there isn't a blocker. Since it reads and operates fine, just a little wonky and verbose in JAWS.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, please take out the nav for the 'container' Tabs. See: #2272
I'm not quite sure what 'default' Tabs are, but typically you would want to leave landmark design, like adding a nav, main, etc, up to the page author. (i.e. I think the nav should probably be taken out of 'default' Tabs, too).
className={classes.tabs} | ||
role={role} | ||
onScroll={this.handleScroll}> | ||
<button |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- these overflow buttons should be
aria-label
ed - when using JAWS/NVDA/VoiceOver to navigate the tablist with these buttons the tabs (and their selection) isn't announced. Maybe an
aria-live
would help here? - during testing only the 'rendered when selected' tab is read when selected, which makes sense since the DOM is being updated. Unless there's something I'm forgetting we probably want the tab panels to be read when the tab is selected in the tablist. Like mentioned above
aria-live
in conjunction witharia-atomic
could be cool here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Worth noting as well here -- and maybe something we document somewhere -- is that NVDA and VO totally ignore the overflow buttons altogether and just let the user access the whole tablist. This seems like what we want (so of course JAWS doesn't do that lol)
Not a blocker, just wanted to note that here for future docs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just FYI we can add aria-live
to the tab content container by default but the user can render any content they would like so we won't be able to account for that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Before you code any live regions, please watch Sarah Higley’s Live Regions talk. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I won't be adding that attribute, due to the users ultimately having control over the tab content as I mentioned previously!
this.handleOverflowNavButtonClick(event, { direction: 1 }) | ||
}> | ||
<ChevronRight16 /> | ||
</button> | ||
</div> | ||
{tabContentWithProps} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overflow tabpanel content is always read by my readers -- even if it isn't expanded. We could either fix that and have text read truncated and call out the expand button, or hide the button (consider it only a visual affordance) and just have screen reader users have immediate access to all the tab panel content. 🤔
The as-is is kinda weird in that the screen reader user can read the whole panel, and then sees the expand button that technically does nothing for them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this behavior is also carried over from the existing non mobile tabs. given that mentioned a couple of options, what would be the recommendation here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is truly truncating the text -- not just visually an option? If not we could just set aria-hidden
on the show more button
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
by "show more button" are you referring to the overflow buttons? if so, would aria-hidden
clash with the suggestion to add aria-label
s above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I meant the show more button for the tab panel content.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that's part of the code snippet and is separate from the tabs component
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, of course!
Just a general question for the review team here -- are we testing on mobile? I have an Android and can test there if that's helpful. @tw15egan @aagonzales |
Ha what a logical question @dakahn. No i didn't actually try it on a mobile device but I will now! |
@dakahn I usually test mobile in-browser unless it's a mobile-specific fix. Will verify on a device with this one 👍 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 ✅ 🎉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Skeleton looks good now!
FYI This ended up being a breaking change - Really would have appreciated a deprecation notice on the |
…ttons (carbon-design-system#6410)" This reverts commit cdc96da.
…ttons (carbon-design-system#6410)" This reverts commit cdc96da.
…ttons (carbon-design-system#6410)" This reverts commit cdc96da.
…ttons (carbon-design-system#6410)" This reverts commit cdc96da.
…ttons (carbon-design-system#6410)" This reverts commit cdc96da.
Closes #4758
This PR removes the dropdown tabs variant at small screen sizes and replaces it with a scrollable tab list and overflow nav buttons. Light component variants are also introduced. The overflow indicator fade effect is similar to the modal body and code snippet overflow indicator fade effect (and faces the same limitations when using CSS custom properties, and the same limitations with transparent gradients in Safari #4769)
Changelog
New
Removed
Testing / Reviewing
Confirm the overflow tabs and overflow container tabs match the component spec in visuals and behavior. The component should be navigable with the keyboard and with the mouse. When using the overflow nav buttons, reaching the end of the tab list will hide the overflow nav button at that edge and move focus to the currently selected tab