-
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): add unstable_Tabs component #6803
feat(Tabs): add unstable_Tabs component #6803
Conversation
Deploy preview for carbon-elements ready! Built with commit 79616df |
Deploy preview for carbon-components-react ready! Built with commit 79616df https://deploy-preview-6803--carbon-components-react.netlify.app |
e2ec52e
to
98a0632
Compare
Do we need to enable a feature flag to get the styles to render properly? Figured if it was under the |
Could we add the mobile tabs version as a variation instead of it being experimental? Was looking at this real quick and was wondering if the changes were one such that we could support both or not 🤔 |
I don't think the is it worth adding a variation if the dropdown tabs are going to be removed? we would just be adding a prop to render a whole new component, just within the |
98a0632
to
6674eeb
Compare
I think the idea is to try and figure out how to ship In the React component, we could have the default tabs class name be the new variant and then other teams could still use the old class names. In v11, we'd have them be aliases of each other. |
the new variant is technically a breaking change though right? that's why I thought it made sense to roll back to the dropdown variant until v11 and release the new variant as unstable |
@emyarod I think the breaking change would be for consumers of Carbon styles if we changed ones like |
I see, so what should we do in that case where it's not a breaking change for React but it will be breaking for Carbon implementations in other frameworks? |
@emyarod this is where I was asking if we could have an alternate class name that we use by default in |
if this is just about avoiding the this is technically a breaking change since we are also changing the |
b78a458
to
846d7ad
Compare
Yeah, I think the ideal would be that we don't have It'd be great if we could have an adapter for this under Let me know what you think 👀 |
ok I see this change will be pretty involved to make sure no regressions are introduced. after adding this backwards compatibility we just need to make sure to remove it in the next major version |
…ystem#6843)" This reverts commit 87d02b7.
…ttons (carbon-design-system#6410)" This reverts commit cdc96da.
846d7ad
to
79616df
Compare
opened #6928 if we prefer to go that route, not sure if it's as clean or if it contains any regressions compared to this PR but let me know what you think |
closing in favor of #6928 |
Closes #6798
Ref #4758
This PR restores the dropdown tabs and moves the new mobile Tabs variation to the
unstable_Tabs
componentChanged
div
inTabs
andunstable_Tabs
. This saves us the trouble of renaming classes individually and only requires us to remove the added parent class in our stylesheet, once the scrolling tabs variant replaces the default. The div can be reverted to a fragment at that time as wellTesting / Reviewing
Ensure the dropdown tabs and new mobile tabs appear and function correctly