-
Notifications
You must be signed in to change notification settings - Fork 4.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: Create stories for DS color scheme #13099
feat: Create stories for DS color scheme #13099
Conversation
✅ Deploy Preview for ethereumorg ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
5358ff7
to
1699afc
Compare
1699afc
to
300be0c
Compare
@pettinarip I am not a fan of placing this in |
Wohoo! this is great man. cc @nloureiro look at this 💪🏼
Good point and good idea. Lets do that. It will be more organized. |
Awesome!!! Just a heads up, we probably will do a big update on the colors alongside the homepage EPIC. |
@pettinarip @nloureiro as an FYI, I am hoping to flip this over to creating the stories via MDX. For Example, check out the Narmi Design System Color Stories which have the MDX doc of different groups of colors in their DS, which in turn generates the individual stories for each group. This would probably be better discussed with overall consideration in adding docs to the other stories from the DS. |
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 @TylerAPfledderer, left one potential improvement.
<Stack direction="column"> | ||
<Text size="sm">{value}</Text> | ||
<Text size="sm"> | ||
{color}.{tokenKey} |
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.
Good job @TylerAPfledderer. I like how we are displaying the colors in the Primitives
case.
Two things I think would be useful for devs to display on the semantic tokens story:
- code example or semantic token usage
- primitive color reference
For example, for the primary "base" token we could add:
- how it would be used in code:
primary.base
- the primitive color used by this token:
blue.500
ororange.500
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.
@pettinarip will do! I think I have an approach in mind.
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.
@pettinarip I updated the labelling of the semantic tokens. It got a little tricky trying to come up with a good way to extract the foundation tokens for each to make as labels. I don't want to try and spend too much time trying to come up with some generator that would get messy in this PR.
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.
Sure. Cool, it looks more useful now with the actual tokens that we use on the code, thanks!
1f2059c
to
f78c5ce
Compare
color: string | ||
children: ReactNode | ||
}) => { | ||
return ( |
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.
@TylerAPfledderer Getting this when I look at the primitives in Dark Mode. Otherwise this all looks great!
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.
@wackerow yea... they should not be view in dark mode. Only light mode. I did not create this story with that in mind.
Need to figure out how I can update the SB config to force a color mode! That's been a back-burner task with the Chakra provider
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.
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 didn't think that would make sense because we don't see the orange shades in light mode.
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.
Maybe here I could more easily show the blues or the oranges based on color mode. I'll take a look at that.
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.
Not sure I followed you there @TylerAPfledderer.
IMO we should not do anything here in terms of color mode, since the purpose of this story is to list the color palette we use. The color mode should only be reflected within the semantic tokens story.
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.
Then I would try and disable the color mode toggle on the toolbar. We don't want to be switching modes because we just want to view everything at the same time (mimicking what it looks like in the figma file)
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.
Overall I think this is ready to pull in as is, and can make any improvements in other PRs. Any objections to pulling this in @pettinarip @TylerAPfledderer @wackerow?
Description
Adds a set of stories to Storybook, showcasing the Color scheme.
Only shows the base color tokens and the semantic colors.
Does not create Chromatic snapshots