Skip to content
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.

Sign up for GitHub

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

Jump to bottom

fix(data-viz): ensure color palette updates on theme change #2007

Merged
merged 5 commits into from
Dec 2, 2020
Merged

fix(data-viz): ensure color palette updates on theme change #2007

merged 5 commits into from
Dec 2, 2020

Conversation

tw15egan
Copy link
Collaborator

#1936

Fixes an issue where the color palette was not updated when the theme was changed

Changelog

Changed

  • useEffect hook to update the color values when the theme is changed, not just when the dropdown is changed

@tw15egan tw15egan requested a review from a team as a code owner November 24, 2020 15:31
@vercel
Copy link

vercel bot commented Nov 24, 2020 •
edited
Loading

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/carbon-design-system/carbondesignsystem/478h8cmqw
✅ Preview: https://carbondesignsystem-git-color-palette-fix.carbon-design-system.vercel.app

@tw15egan tw15egan requested a review from jeanservaas November 24, 2020 17:41
jeanservaas
jeanservaas reviewed Nov 24, 2020
View reviewed changes
Copy link
Collaborator

@jeanservaas jeanservaas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks for getting on this! Just a couple of things:
That dropdown (for the color groupings) to the right of the tabs should also have a white (not g10 background)

image

The alert palette also changes from light to dark, right now those are only the light values on both of the tabs and green value is wrong for the light:

image

image

Should be:
image

@tw15egan
Copy link
Collaborator Author

@jeanservaas updated, although I noticed #198038 (rather than #78a9ff) seems like the correct hex for Green 60.

@alisonjoseph alisonjoseph mentioned this pull request Nov 30, 2020
Closed
39 tasks
emyarod
emyarod approved these changes Dec 1, 2020
View reviewed changes
Copy link
Member

@emyarod emyarod left a 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 pending visual review

@tw15egan tw15egan requested a review from jeanservaas December 1, 2020 20:45
jeanservaas
jeanservaas approved these changes Dec 2, 2020
View reviewed changes
Copy link
Collaborator

@jeanservaas jeanservaas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

boom

@kodiakhq kodiakhq bot merged commit c7806e9 into carbon-design-system:master Dec 2, 2020
natashadecoste pushed a commit to natashadecoste/carbon-website that referenced this pull request May 19, 2021
@tw15egan @jeanservaas @natashadecoste
fix(data-viz): ensure color palette updates on theme change (carbon-d…
6951e4a 
…esign-system#2007)

* fix(data-viz): ensure color palette updates on theme change

* fix(data-viz): update alert colors

Co-authored-by: Alessandra Davila <[email protected]>
Co-authored-by: jeanservaas <[email protected]>
natashadecoste pushed a commit to natashadecoste/carbon-website that referenced this pull request May 19, 2021
@tw15egan @jeanservaas @natashadecoste
fix(data-viz): ensure color palette updates on theme change (carbon-d…
52c0d72 
…esign-system#2007)

* fix(data-viz): ensure color palette updates on theme change

* fix(data-viz): update alert colors

Co-authored-by: Alessandra Davila <[email protected]>
Co-authored-by: jeanservaas <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jeanservaas jeanservaas jeanservaas approved these changes

@aledavila aledavila aledavila approved these changes

@emyarod emyarod emyarod approved these changes

Assignees
No one assigned
Labels
status: ready to merge 🎉
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@tw15egan @emyarod @aledavila @jeanservaas