feat(react): add IconTab component with size prop #10301
Conversation
|
✔️ Deploy Preview for carbon-react-next ready! 🔨 Explore the source changes: cd2612c 🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-react-next/deploys/61e1b8e1cdab6f0008b8dd2e 😎 Browse the preview: https://deploy-preview-10301--carbon-react-next.netlify.app |
|
✔️ Deploy Preview for carbon-components-react ready! 🔨 Explore the source changes: cd2612c 🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-components-react/deploys/61e1b8e19b24bd00089fa146 😎 Browse the preview: https://deploy-preview-10301--carbon-components-react.netlify.app |
|
✔️ Deploy Preview for carbon-elements ready! 🔨 Explore the source changes: cd2612c 🔍 Inspect the deploy log: https://app.netlify.com/sites/carbon-elements/deploys/61e1b8e11391850008d46a80 😎 Browse the preview: https://deploy-preview-10301--carbon-elements.netlify.app |
There was a problem hiding this comment.
A couple small things, otherwise looks great!
The question of adding this as a component vs placing these classes behind prop(s) on Tab lingers with me. What if it was a single prop on Tab that did both?
iconSize: PropTypes.oneOf([16, 20])
Without a default we can infer that .#{$prefix}--tabs__nav-item--icon should only be applied if we have a value for iconSize. This would also toggle .#{$prefix}--tabs__nav-item--icon--{default || lg} of course.
I'm open to either approach, curious what do you think?
|
|
||
| IconTab.propTypes = { | ||
| /** | ||
| * Provide an icons to be rendered inside of `IconTab` as the visual label for Tab. |
There was a problem hiding this comment.
| * Provide an icons to be rendered inside of `IconTab` as the visual label for Tab. | |
| * Provide an icon to be rendered inside of `IconTab` as the visual label for Tab. |
| */ | ||
| className: PropTypes.string, | ||
| /** | ||
| * size of IconTab, default is intended for use with size 16 icons and lg is intended for use with size 20 icons. |
There was a problem hiding this comment.
| * size of IconTab, default is intended for use with size 16 icons and lg is intended for use with size 20 icons. | |
| * Specify the size of the `children` icon. `default` is intended for use with size 16 icons and `lg` is intended for use with size 20 icons. |
|
|
||
| .#{$prefix}--tabs__nav-item--icon--lg { | ||
| // had to use exact pixels because there is no way to get to 48px h/w without it. | ||
| padding: 13px 14px; |
There was a problem hiding this comment.
I think px values are generally converted to rem elsewhere in the codebase
| padding: 13px 14px; | |
| padding: rem(13px) rem(14px); |
| <Tabs> | ||
| <TabList aria-label="List of tabs"> | ||
| <Tab disabled> | ||
| <IconTab size="lg" disabled> | ||
| <Monster20 /> | ||
| </Tab> | ||
| <Tab> | ||
| </IconTab> | ||
| <IconTab size="lg"> | ||
| <Corn20 /> | ||
| </Tab> | ||
| <Tab> | ||
| </IconTab> | ||
| <IconTab size="lg"> | ||
| <Bat20 /> | ||
| </Tab> | ||
| </IconTab> | ||
| </TabList> |
There was a problem hiding this comment.
Would it be possible to do <IconTabs size="lg"> so that we could define the size at the top-level of the component?
We could have the .cds--tabs__nav-item--icon class be something like:
.cds--tabs__nav-item--icon {
min-height: var(--cds-tab-height, /* add whatever default height is */);
}And in tabs set that CSS Custom Property depending on the prop passed in
| /** | ||
| * If using `IconTab`, specify the size of the icon being used. | ||
| */ | ||
| iconSize: PropTypes.oneOf(['16', '20']), |
There was a problem hiding this comment.
| iconSize: PropTypes.oneOf(['16', '20']), | |
| iconSize: PropTypes.oneOf(['default', 'lg']), |
I believe we should use the size names from other components to match convention there, let me know if I'm off 👍
Closes #9991
Adds
IconTabwhich just extendsTabwith an added size prop for "default" and "lg" for 16px icons and 20px icons respectively.I wasn't sure how to go about adding the
sizeprop for Tab since it is only intended to be used with icons as the label. I addedIconTabwith the hope that it would make its intended use clear.Changelog
New
IconTabcomponentsizepropTesting / Reviewing
Go to
carbon-react-nextstorybook and verify that the individual Tabs forIcon onlyandIcon 20 onlymatch the specs above. 40x40 for 16px icons and 48x48 for 20px icons.