feat(vertical-tabs): new tab variant #16701
Conversation
✅ Deploy Preview for v11-carbon-react ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for carbon-elements ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
This is looking good! My one question, which may be more for design, is around the 25% width. I think this might work as a default, but I can see scenarios where someone might need to align/wrap this on the grid. Perhaps it needs a width prop where you can set a width, or leave it as 100% so the user can place it within their own container? We solved a somewhat similar problem for grid-aligned contained tabs with a combination of grid components and a fullwidth prop. https://react.carbondesignsystem.com/?path=/docs/components-tabs--overview#tabs-and-the-grid---fullwidth-prop |
There was a problem hiding this comment.
Some scrollIntoView related observations/questions:
It's possible to select a tab and not be able to see it. The focus ring is obscured by the gradient which might be an accessibility failure?
CleanShot.2024-06-07.at.10.43.33.mp4
When a tab is selected and the browser size changes the tab isn't visible in some cases - both in the vertical config and the smaller horizontal config
CleanShot.2024-06-07.at.10.47.01.mp4
| // un-hide hidden Tab Panels to get heights | ||
| refs.current.forEach((ref) => { | ||
| if (ref) { | ||
| ref.hidden = false; | ||
| } | ||
| }); |
There was a problem hiding this comment.
If I understand the intent here, I think you could avoid having to unhide them like this by using useLayoutEffect. It's specifically geared towards logic that needs to measure the dom.
OR maybe this justifies having a wrapper component around the tabs and the tabpanels. That way we wouldn't have to measure anything and it could be done with CSS. Honestly I'd be up for that approach over measuring with js to match heights. Are there any downsides of a wrapper element that you can think of?
There was a problem hiding this comment.
I thought the useLayoutEffect would work here as well!
When trying this:
useLayoutEffect(() => {
refs.current.forEach((ref) => {
console.log(ref?.offsetHeight);
});
});
^ i would only get the following printed... the only visible height 48 and the rest 0
The only side effect of a wrapper element that I can see is possibly a bit of a structure change, but there would be a lot of benefits layout wise for vertical tabs. This could also solve the issue @alisonjoseph brought up where I am using percentages.
There was a problem hiding this comment.
just confirming the above that adding a wrapper with styles such as:
.#{$prefix}--tabs-wrapper {
display: flex;
}
then would remove the need for using javascript and css would automatically handle the heights. If both the default and vertical tabs have this wrapper though then there would need to be some javascript to handle when to apply display: flex or not to handle the different layouts for vertical and default
|
@alisonjoseph the design intent is for the tablist and tab panel to be grid-aligned @tay1orjones i think that you are right in that is is (or will be) an accessibility violation if focus is obscured so we must keep the focused/active tab in view. |
|
closing this PR in favor for: #16738 |
#16305
Adds a new vertical variant to tabs
Changelog
New
Changed
overflow-x: hiddento default Tabs ... fixed an issue where an unwanted horizontal scroll would appear if the length of the tabs is too longOther notes:
float: inline-startto align the vertical tabs. This avoid wrapping the component in<Grid>, which causes complexities when going betweensmand other breakpoints, but I knowfloatisn't really used any morestyle.height = ...for each TabPanel to set what the height of the largest height for each. I am not noticing any flashing, but could be a concernsmbreakpoint should be default tabs or contained tabsTesting / Reviewing
{{ Add descriptions, steps or a checklist for how reviewers can verify this PR works or not }}