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(tab): introduce fixed variant #4431

Merged
merged 6 commits into from
Oct 25, 2019

Conversation

asudoh
Copy link
Contributor

@asudoh asudoh commented Oct 23, 2019

The new variant is enabled with .bx--tabs--fixed class.

Fixes #1329.

Changelog

New

  • .bx--tabs--fixed class, which enables the new tab variant.
  • A React prop to enable such class.

Testing / Reviewing

Testing should make sure our tabs component is not broken.

The new variant is enabled with `.bx--tabs--fixed` class.

Fixes carbon-design-system#1329.
@netlify
Copy link

netlify bot commented Oct 23, 2019

Deploy preview for the-carbon-components ready!

Built with commit e0f760f

https://deploy-preview-4431--the-carbon-components.netlify.com

@netlify
Copy link

netlify bot commented Oct 23, 2019

Deploy preview for carbon-elements ready!

Built with commit e0f760f

https://deploy-preview-4431--carbon-elements.netlify.com

@netlify
Copy link

netlify bot commented Oct 23, 2019

Deploy preview for carbon-components-react ready!

Built with commit e0f760f

https://deploy-preview-4431--carbon-components-react.netlify.com

@netlify
Copy link

netlify bot commented Oct 23, 2019

Deploy preview for the-carbon-components ready!

Built with commit 34851f8

https://deploy-preview-4431--the-carbon-components.netlify.com

@netlify
Copy link

netlify bot commented Oct 23, 2019

Deploy preview for carbon-elements ready!

Built with commit 34851f8

https://deploy-preview-4431--carbon-elements.netlify.com

@netlify
Copy link

netlify bot commented Oct 23, 2019

Deploy preview for carbon-components-react ready!

Built with commit 34851f8

https://deploy-preview-4431--carbon-components-react.netlify.com

Copy link
Member

@emyarod emyarod left a comment

Choose a reason for hiding this comment

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

it looks like the div inside flexbox container situation also occurs in our vanilla docs (related 621be60)

.component-example__live--rendered > div is a div inside of a flexbox so it no longer has width: 100% by default, and instead it shrinks to fit the content, which looks like this

image

@@ -5,7 +5,7 @@
LICENSE file in the root directory of this source tree.
-->

Copy link
Member

Choose a reason for hiding this comment

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

we may need unique IDs for the examples otherwise the tabs will control the content for all examples

.#{$prefix}--tabs--fixed
.#{$prefix}--tabs__nav-item:hover:not(.#{$prefix}--tabs__nav-item--disabled) {
@include carbon--breakpoint(md) {
background-color: $hover-ui;
Copy link
Contributor

Choose a reason for hiding this comment

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

I know this is the token the spec has, but for some reason the color it's receiving is #e5e5e5 instead of #cacaca, so the hover color is lighter and it should be darker. Not sure why the color doesn't match the token though. Is this the right token? @aagonzales
Screen Shot 2019-10-23 at 11 31 41 AM

Copy link
Member

@aagonzales aagonzales Oct 23, 2019

Choose a reason for hiding this comment

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

good catch, it will need to use a different token. Use$hover-selected-ui (even though its not "selected" it gets it the correct color) 🤷‍♀

@jnm2377
Copy link
Contributor

jnm2377 commented Oct 23, 2019

Just a few things:

  • I'm not seeing the tab content background color in the React Storybook.
  • Could we move the React example to be it's own story rather than a knob, like our other component examples? I feel like knobs are often hard to find for users/most people don't know to look there.

Copy link
Member

@aagonzales aagonzales left a comment

Choose a reason for hiding this comment

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

Bugs

  • height should be 48px / 3rem
  • border top should be 2px

image

A few design tweaks from the original spec we want to change:

  • remove border-bottom from all the tabs
  • remove border's to the left and right of the selected tab

image

In the example:
Can we add a background color to some-content area - $ui-01

@asudoh
Copy link
Contributor Author

asudoh commented Oct 24, 2019

Thanks @aagonzales @emyarod @jnm2377 for reviewing!

  • Removed borders of:
    • Bottom one entierly
    • Left/right ones for selected tab
  • Reduced the (top) border size of selected tab
  • Changed the background color of selected tab
  • Increased the height of the tab for the new variant
  • Fixed ID duplication in vanilla
  • Styled React tab panel
  • Fixed width and top margin of vanilla tab panel
  • Made different story for the new variant. Though we have several knobs for different variants (e.g. light variant), I see @jnm2377's point wrt difficulty to find out

Copy link
Member

@emyarod emyarod left a comment

Choose a reason for hiding this comment

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

the vanilla IDs still seem to be shared since the examples are controlling each other, otherwise looks good to me pending design review

Copy link
Member

@aagonzales aagonzales left a comment

Choose a reason for hiding this comment

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

Nice! They're looking good, thanks for updating. Just a few bugs still:

  • Height of tabs should be 48px/3rem

  • There's a weird delay in state change when the previous selected is transitioning to unselected
    Oct-24-2019 10-02-25

  • On hover both borders should disappear, only one is currently
    Oct-24-2019 10-10-19

@jnm2377
Copy link
Contributor

jnm2377 commented Oct 24, 2019

hmm looks like the state delay is an existing bug. It happens in the default tabs as well @aagonzales @asudoh

Copy link
Contributor

@jnm2377 jnm2377 left a comment

Choose a reason for hiding this comment

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

Other than the hover border, LGTM. I would be ok with opening a separate issue/PR for the selected state delay since it's technically a different existing bug/out of scope for this PR, but also would be ok if you wanted to include a fix for it in this PR since you're already working on tabs. 👍

@asudoh
Copy link
Contributor Author

asudoh commented Oct 25, 2019

@aagonzales @emyarod Good catches - Ensured 3rem height, removed right border for selected tab, and resolved duplicate tab panel target. For the detailed animation wrt selected state, I see a performance issue in the Storybook setup. Fixed it that seems to solve or at least alleviate the problem. If the problem still persists I think it's a good idea to defer it, as @jnm2377 saw that it's not a new issue.

Copy link
Member

@aagonzales aagonzales left a comment

Choose a reason for hiding this comment

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

🎉 awesome! ship it!

@asudoh asudoh merged commit 2811a2c into carbon-design-system:master Oct 25, 2019
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.

[Tabs] Add a secondary style
4 participants