fix: remove color-context JS in favor of color-scheme and light-dark()
#2138
Conversation
|
✅ Deploy Preview for red-hat-design-system ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
|
Size Change: -3.4 kB (-1.65%) Total Size: 203 kB
ℹ️ View Unchanged
|
|
Really impressive!
|
Yes looks like we should.
Yes need to go through the changes I've made here with a fine tooth comb and ensure the changes in the element sheets can't be replaced with the computed value already present in
That was my take away with the changes made, that there wasn't anything fundamentally changing for the user. |
Thinking about this more, what if someone has written a 3rd party component that relies on |
|
I'm preemptively invalidating that user's abuse of the context system. |
I'm even just talking about this: If we say surface no longer provides the means to satiate |
These 3 things are not "gotchas" from my position. If anything,
|
could be, but i sincerely doubt this is a real case in the wild
100%. they should be removed from the context system and treated as themes
the reasons we find it necessary are for contrast in specific scenarios: a better solution is colour transforms in oklch |
Yeah, if you are just suggesting the removal of |
This might be a dumb question, but if we don't add it as a color palette, does this prevent us from using it as a surface token in an element?
Agreed. We may have a need for more than just light and dark theme in the future, but right now, designers are focusing on getting at least those two themes working for stuff like docs.redhat.com. Better to tackle any saturated or base theming later.
Should |
| color: | ||
| light-dark(var(--rh-color-text-primary-on-dark, #ffffff), | ||
| var(--rh-color-text-primary-on-light, #151515)); |
There was a problem hiding this comment.
| color: | |
| light-dark(var(--rh-color-text-primary-on-dark, #ffffff), | |
| var(--rh-color-text-primary-on-light, #151515)); | |
| color: var(--rh-color-text-primary); |
There was a problem hiding this comment.
this file should
- live in the tokens repo
- be generated through style dictionary build there
- have two flavours:
:host([color-palette=whatev])for shadow and[color-palette=whatev]for light (not super sure about the lightdom one...)
zeroedin
changed the title
color-scheme and light-dark()color-scheme and light-dark()
What I did
TODO
rh-surfacetests: Since context is removed, JS eventing is no longer present, these tests need to be re-evaluated to determine how we would approach them. Surface is now only a passive element that sets a parentcolor-scheme.I'm unsure how much testing we'd want to do here as it would be mainly style testing which we've tended to avoid doing.rh-badgetests in this component and in any component tests that include testing of CSS properties contained incolor-scheme.css.will need to be updated to include those CSS in the test fixture output.Testing Instructions
Notes to Reviewers
Some components had their CSS modified to accommodate this approach. Avatar and Breadcrumbs used the
.darkconsumer class to swap images. I took a couple of different approaches in this PR.In Avatar, the default SVG was swapped in the render() based on context; in this PR, it now styles the SVG internals using
light-dark()and just loads a single SVG in the render(), no swapping necessary.Breadcrumbs used data URI SVG and swapped the URI based on the
.darkclass. Unfortunately, we can not uselight-dark()to swap a background image, so instead, I used the data URI SVG as a mask and then usedlight-dark()to style the background color the mask cuts out. Same result, different approach.Other than that, the addition of the
color-scheme.cssis necessary. My assumption is that maybe this file could be served by the tokens repo as part of global.css or as a separate file.Please think about these changes and future-forward feature sets that will be here soon (i.e., style queries). Am I missing something? Is there potential where this doesn't work?
In this PR, I personally did not find a current use case that it doesn't work, only places we needed to take a slightly different approach mentioned above.