-
Notifications
You must be signed in to change notification settings - Fork 185
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(core): Colors, themes & more design updates #261
Conversation
Deploy preview for carbon-charts ready! Built with commit c32b6da |
Looking good 🎉 I'll close my draft since you've got all the high points covered |
Cool. We should find a way to collab on PRs, since I could've expanded on yours |
Question, so in Carbon we do visual reviews in the PR thread. This thread might get really long because all the charts updates are in one PR. Is there a different place you guys do visual reviews before merge? Or should we split this PR into a few parts so we can merge the charts one by one as soon as it is ready? |
@shixiedesign This PR will mostly focus on some of the obvious or immediate changes we're going to merge (once colors are approved we will probably go ahead an merge this, then open up separate PRs for the nitty gritty of the rest of the component related updates) Therefore, you're welcome to share your review here 🙂 |
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.
Visual feedback on the color themes:
-
[Theme 1] all looks good except 1 color, Purple 80 should be Purple 30. My mistake in the spec image!
-
[Theme 3] is it possible to implement the dark theme? For light ui, theme 1 & 2 are sufficient for the time being I think. Unless you know more use cases. For dark ui, let's use this color progression:
@shixiedesign Added all 3. For [Theme 2] that you sent, I added another variation teal Here are the themes: |
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.
LGTM code wise 👍
Can we add a Charts page to the main site (possibly linking to this site) so this is actually discoverable? |
@jeoffw yes please 😃 |
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.
👍 Thanks!
@zvonimirfras @cal-smith I changed the export We can explore the possibility of "themes" at some point, but I'd like to call the newly added service color palettes. Feel free to merge if there are no objections 💯 |
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.
Makes sense, themes are definitely a larger topic than just the colours ... Everything looks good to me 👍
Updates
#246, #252, #254, #255
Closes #243