-
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): allow dismissable tabs to have descriptive icons #13830
feat(Tabs): allow dismissable tabs to have descriptive icons #13830
Conversation
✅ Deploy Preview for carbon-components-react ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
✅ Deploy Preview for carbon-elements ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
✅ Deploy Preview for carbon-components-react ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
✅ Deploy Preview for carbon-elements ready!
To edit notification comments on pull requests, go to your Netlify site settings. |
…criptive-icon-tab-layout-helpercomponent
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to go! 💥
…845-tabs-provide-dismissable-tabs-with-descriptive-icon-tab-layout-helpercomponent
…on-tab-layout-helpercomponent' of github.com:francinelucca/carbon into 12845-tabs-provide-dismissable-tabs-with-descriptive-icon-tab-layout-helpercomponent
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me. Unrelated, but I noticed that on the disabled state for dismissable tabs, the close icon still gets a pointer mouse cursor. It should be the disabled mouse cursor. I can open a separate issue for this if you want.
@laurenmrice I made the cursos a pointer since the close button is actually clickable because disabled tabs are dismissable (Ref: #13529 (comment)) I can definitely change the cursor to a disabled mouse if that's the intended behavior though 🤔 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tested locally and LGTM!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍 ✅
@francinelucca Yes, it is the intended behavior that if a dismissable tab is disabled, you can not actively close that tab by using the close icon, so the mouse indicator should be the disabled one. I would say if someone has a different use case for this they could change it on their end. |
|
…845-tabs-provide-dismissable-tabs-with-descriptive-icon-tab-layout-helpercomponent
…criptive-icon-tab-layout-helpercomponent
…criptive-icon-tab-layout-helpercomponent
…criptive-icon-tab-layout-helpercomponent
Closes #12845
Changelog
New
-Dismissable tabs with icons story + VRT
$prefix}--tabs__nav-item--icon-left
classChanged
Testing / Reviewing
Check that deployment story matches spec
*Added a temporary "contained" toggle to switch between line/contained tabs for review purposes
TODO: