-
Notifications
You must be signed in to change notification settings - Fork 14.1k
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: storybook for Icon component #10515
Conversation
export const SupersetIcon = () => { | ||
return ( | ||
<> | ||
{Object.keys(iconsRegistry).map(iconName => ( |
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.
A bit of a nit, do you think we could display these icons in such a way that they take up a bit less space? I've liked a design like this lately: https://airbnb.io/lunar/?path=/story/icons--icon-gallery
{Object.keys(iconsRegistry).map(iconName => ( | ||
<> | ||
{iconName}: <br /> | ||
<Icon |
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.
Should we add a knob for size as well?
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.
Hmm... I don't see a size
prop in use in the codebase, or explicitly supported by the Icon component.
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.
Otherwise, I'm down to add knobs for all the things
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.
width and height are svg props which can be passed into the Icon component, along with viewBox to properly scale to svg.
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.
perhaps we should consider abstracting width, height, and viewBox away into a size
prop? But regardless, that's for a future pr. This is good as is then
🏷 svg |
db451a8
to
2f8c588
Compare
Oh the label bot is working again!? Came back to life!?!?!! About the storybook, I'd go for much more density, maybe in a grid? Small text? |
Ok, icons are now in a nice little grid... sorry for being a lazy bum :P And hooray label bot! |
I also sorted the icons alphabetically, so it doesn't hurt people's brains. |
🏷️ storybook |
🏷 storybook |
WHAT? bot only listens to you!? |
🗑️ 🏷️ storybook |
🗑🏷 svg |
🏷️ .ui |
That's super offensive. Bot revolts against its maker. |
I'll go tag some random PR I'm NOT the author of to see if it works there. |
🏷️ css |
* storybook for Icon component * fixing webpack aliases * linting ✨ * Icons are now in a nice little grid. * lint * EOF fix for alert.txt. Ugh.
* storybook for Icon component * fixing webpack aliases * linting ✨ * Icons are now in a nice little grid. * lint * EOF fix for alert.txt. Ugh.
SUMMARY
Adds a storybook entry for the Icon component, and puts that Icon component in a folder.
Sadly, the knobs for the color palette are relying on the
SupersetTheme
... I was having some issues gettingwithTheme
to work here, but I'd rather use theThemeProvider
to do this. I'll give that another shot later, as it'll be handy in other stories, I'm sure.BEFORE/AFTER SCREENSHOTS OR ANIMATED GIF
TEST PLAN
ADDITIONAL INFORMATION