Skip to content
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): vertical variation with inner grid #16738

Conversation

ariellalgilmore
Copy link
Member

@ariellalgilmore ariellalgilmore commented Jun 11, 2024

Closes #16305

Adds a new vertical variant to tabs
Screenshot 2024-06-05 at 10 54 44 AM

Changelog

New

  • TabListVertical component
    • will scroll selected <Tab> into view on resize and when using mouse/keyboard
  • new vertical and height props to Tabs
    • the vertical Tabs variant wraps a Grid around the component except for at sm breakpoint
  • Tab new update to add ellipsis with Tooltip only on vertical variant
  • TabPanel still needs to account for changing heights if the adopter doesn't give the Tabs a set height

Changed

  • added overflow-x: hidden to default Tabs ... fixed an issue where an unwanted horizontal scroll would appear if the length of the tabs is too long

Other notes:

Testing / Reviewing

{{ Add descriptions, steps or a checklist for how reviewers can verify this PR works or not }}

Copy link

netlify bot commented Jun 11, 2024

Deploy Preview for v11-carbon-react ready!

Name Link
🔨 Latest commit 5ba02e1
🔍 Latest deploy log https://app.netlify.com/sites/v11-carbon-react/deploys/6682fc473ab811000857a868
😎 Deploy Preview https://deploy-preview-16738--v11-carbon-react.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

netlify bot commented Jun 11, 2024

Deploy Preview for carbon-elements ready!

Name Link
🔨 Latest commit 5ba02e1
🔍 Latest deploy log https://app.netlify.com/sites/carbon-elements/deploys/6682fc476fbf030008e646bb
😎 Deploy Preview https://deploy-preview-16738--carbon-elements.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@alisonjoseph
Copy link
Member

This is looking good!

Couple things

  • we will need to make sure we have tests added (AVT, VRT, unit)
  • we reuse the storybook stories for the live demo on the website, so will want to probably just show the tabs with the height set, and also make sure the controls are enabled for that story. (we can wait to update this until after it has been reviewed fully / by design etc)

@alisonjoseph alisonjoseph requested a review from tay1orjones June 13, 2024 16:27
@kingtraceyj
Copy link
Member

@ariellalgilmore looks good to go from design! 💥

Copy link
Member

@alisonjoseph alisonjoseph left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

still reviewing, but linked to a recent PR that went in that removed all those css overrides in favor of importing the Layer component directly into Modal. Thinking we can do the same here with tabs?

@@ -0,0 +1,66 @@
//
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we still need this file? These styles were just removed here #16860 in favor of using the Layer component

I think we might be able to replace the div at line 1641 (tab panel) with <Layer>

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks alison! I saw that PR and when swapping the div to layer it ends up with a white background
Screenshot 2024-06-27 at 5 42 40 PM

Adding the layer around the children works, but....
Screenshot 2024-06-27 at 5 33 49 PM

if it's not in a container Tabs then the text input background will blend with the tabs background
Screenshot 2024-06-27 at 5 34 11 PM

i ended up putting the Layer in the story here since the TabPanel doesn't have access to knowing if it's inside a container tabs or not. Since Tabs doesn't have a parent element it would be hard to determine inside TabPanel with javascript if the container was being applied as a prop

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ohh ok. Regardless where we add the Layer (in the actual component or in the story example) I think we can remove all of the css overrides.

The example for contained tabs breaks I think because we already have a Layer component in the story code. If we remove that and bake it in (as a child of the div) it should work.

I'm on the fence whether that is a fix or a breaking change. If someone is already nesting Layer for contained tabs then it will break. I'd rather is just "works" out of the box, but might be too late to change that.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hmm i guess right now though if people are using the contained tabs with field elements then it could be displaying wrong... since that's whats currently in storybook right now, so I think it might be better to go the route of just having it work out of the box and overridding some layer styles for the non-contained tabs?

Screenshot 2024-06-28 at 8 34 19 AM

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, that might be the best option. Neither is ideal, but like you said TabPanel has no idea which variant of tabs its being used with.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

just pushed! I actually ended up just using css directly which seemed the cleanest code wise and possibly would be easier on the user side if they need to override for any reason

@alisonjoseph
Copy link
Member

I tested locally inside some weird nested grids and it is working as expected. 🥳

*It does break if you try and place it inside a column that spans less than 5, which is probably fine. Could be fun to play with container queries to see we could fix that edge case 🤷‍♀️

@alisonjoseph
Copy link
Member

Screenshot 2024-06-28 at 9 42 59 AM

Seeing some issues with default tabs now

@ariellalgilmore
Copy link
Member Author

Screenshot 2024-06-28 at 9 42 59 AM

Seeing some issues with default tabs now

hmm i'm not seeing that and doesn't seem like Percy is picking it up either? What browser/size were you looking in?
Screenshot 2024-06-28 at 8 26 49 AM

@alisonjoseph
Copy link
Member

@ariellalgilmore in Firefox, but I restarted my browser and its working now... 🙃

Copy link
Member

@tay1orjones tay1orjones left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it a problem that there isn't a way to invoke the truncation tooltip via mouse? I can only get it to pop via keyboard.

CleanShot.2024-06-28.at.14.03.20.mp4

@ariellalgilmore
Copy link
Member Author

@tay1orjones we followed the same guidance as operational tag, so on mouse click it shouldn't show the tooltip, but on hover it will eventually pop up with the browser's tooltip

Screenshot 2024-06-28 at 12 06 13 PM

@alisonjoseph alisonjoseph added this pull request to the merge queue Jul 1, 2024
Merged via the queue into carbon-design-system:main with commit f406632 Jul 1, 2024
22 checks passed
@ariellalgilmore ariellalgilmore deleted the feat/vertical-tabs-with-inner-grid branch July 1, 2024 19:52
@carbon-automation
Copy link
Contributor

Hey there! v11.61.0 was just released that references this issue/PR.

@wkeese
Copy link
Contributor

wkeese commented Aug 1, 2024

Uh oh, this broke the background coloring on all our tabs that use contained. (Even though our tabs are horizontal, not vertical.).

This CSS you added is pretty general, it affects any code using <Tabs contained>, not just vertical tabs.

.cds--tabs--contained~.cds--tab-content>* {
  @include layer-tokens.emit-layer-tokens(2);
}

@ariellalgilmore
Copy link
Member Author

Hi @wkeese, can you share a stackblitz example of what is breaking for you? and maybe some screenshots? Looking at storybook here: https://react.carbondesignsystem.com/?path=/story/components-tabs--contained and it is showing the expected results

@wkeese
Copy link
Contributor

wkeese commented Aug 2, 2024

Yes, I started the discussion on Slack (https://ibm-analytics.slack.com/archives/C2K6RFJ1G/p1722510209140579) and posted an example link there.

@wkeese
Copy link
Contributor

wkeese commented Aug 2, 2024

PS: Moved the discussion to ticket #17096.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Vertical Tab variant
5 participants