feat(Tabs): create scrollable Tabs variant #6928
Conversation
|
Deploy preview for carbon-elements ready! Built with commit 1fc08bc |
|
Deploy preview for carbon-components-react ready! Built with commit 1fc08bc https://deploy-preview-6928--carbon-components-react.netlify.app |
There was a problem hiding this comment.
Hey 👋 I've been watching the progress on this issue, and would like to help where I can to get this released as soon as possible. We have a number of consumers that would like to update to a newer carbon-components release and this is currently blocking them from that.
|
Awesome work getting this up, just noticed a small style issue on the scrollable version in Safari while reviewing.
Also noticed some focus issues with the Dropdown variant, but those were present before the scrollable tabs implementation so that can be fixed in another PR. I don't want to hold this fix up 👍 |
|
Do we need to offer backwards-compat in the React code? It seems like the styles should be sufficient and then we wouldn't need to add conditions for scrollable in several places for |
|
yeah we need to add conditions because the internal component method signatures are changed for the scrollable variant. I don't believe that styles are sufficient to switch between the dropdown and scrollable tab behavior. and I also think it would be best not to call any methods associated with scrollable tabs when the dropdown tabs are being used (or vice versa) |
|
@tw15egan I'm not seeing that issue in Safari, can you add some more details about how I might be able to replicate that bug? |
|
Eh, I'm running the MacOS Big Sur beta and noticed an issue that was fixed in a different issue on the new Safari 14.0.0 but was present on Safari 13.1.2. Chalking it up to that 👍 |
|
I think what I'm not understanding is why this needs to be a variant on the React component side. It seemed like the React component should just use the new styles and shouldn't introduce a variant because the scrollable work is the new default for Tabs. The issue with replacing the styles was that it broke framework implementations, so it seemed like if we had both styles that it would solve that issue for them but then in the React implementation we just use the new styles by default and not worry about adding a variant. |
|
oh I see so we make no changes to our JS and just scope the styles? I guess having the component changes was useful for regression testing but I can remove the backwards compatibility in the JS |
|
@emyarod yeah! Exactly, I think that's all that we need to do to fix for framework implementations 👀 |
emyarod
force-pushed
the
6798-create-scrollable-tabs-variant
branch
from
8b6ef10 to
1fc08bc
Compare
|
Thanks all! |
|
@cal-smith pushed up an |
|
@tw15egan just checked it out, looks like everything is sorted! |
Closes #6798
Ref #4758
This PR namespaces the styles and adds conditionals throughout the Tabs methods and props to support compatibility for both scrollable and non-scrollable/dropdown Tabs variants
Changelog
New
scrollableprop to toggle scrollable Tabs variantChanged
Testing / Reviewing
Ensure the scrollable and dropdown/non-scrollable tabs function as expected (for scrollable refer to current stable storybook, and for dropdown/non-scrollable refer to a Carbon version prior to #6410)