-
Notifications
You must be signed in to change notification settings - Fork 12
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
Update new Charts color palette for better accessibility and contrast #395
Conversation
🦋 Changeset detectedLatest commit: af83823 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
@@ -758,7 +757,6 @@ video { | |||
--surface-lg: #ffffff; | |||
--surface-md: #ffffff; | |||
--surface-xl: #ffffff; | |||
--surface-dash-widget: #ffffff; |
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.
Due to the removal of a style here, I think we may want a major
version update, as to not surprise consumers. Even if the only consumers are us, internally, it'd be nice to follow semver. What are your thoughts?
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.
That seems like a good call to me! I've updated the PR to make it a major version bump instead then.
7b0d66b
to
f69884a
Compare
Deploying tailwind-toucan-base with
|
Latest commit: |
af83823
|
Status: | ✅ Deploy successful! |
Preview URL: | https://b9841d1e.tailwind-toucan-base.pages.dev |
Branch Preview URL: | https://jrobards-chart-color-palette-byig.tailwind-toucan-base.pages.dev |
.changeset/red-schools-complain.md
Outdated
"@crowdstrike/tailwind-toucan-base": major | ||
--- | ||
|
||
Update new Charts color palette for better accessibility and contrast |
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.
nit
Could expand this slightly so that consumers have a bit better idea of what the major, breaking change is:
Update new Charts color palette for better accessibility and contrast | |
This release contains a breaking change by removing `surface-dash-widget` classes. If you were previously using these classes, there is no replacement. Updated the existing Charts color palette for better accessibility and contrast. |
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 for calling that out - I didn't think to update the changeset message too!
I split this into two separate changeset messages, since that seems to be supported in a single 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.
Excellent idea! I hadn't thought of that route. Thanks a bunch 🎉
f69884a
to
8ef7c52
Compare
8ef7c52
to
af83823
Compare
This is a follow-up from these two PRS:
Originally when the new chart palette was designed, it was intended to work alongside the new
surface/dash-widget
color. After some discussion, we've decided it would be better for the new palette to work alongside the previously existing surface colors instead to keep things more consistent.So, this PR updates the chart colors to meet accessibility requirements when they are used alongside the existing surface colors. It also removes the
surface/dash-widget
color, since it is not in use and is no longer needed.