Enable container overrides with CSS custom properties #9441
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mxdvl
changed the base branch from
main
to
darkmode/split-theme-decorator-fix
|
Size Change: -12.6 kB (-2%) Total Size: 696 kB
ℹ️ View Unchanged
|
mxdvl
force-pushed
the
darkmode/container-overrides
branch
2 times, most recently
from
2b30917 to
edf21e1
Compare
abeddow91
previously approved these changes
Nov 9, 2023
mxdvl
force-pushed
the
darkmode/container-overrides
branch
6 times, most recently
from
c6aaa41 to
d8595db
Compare
mxdvl
dismissed
abeddow91’s stale review
There have been significant changes since the review
this enables overriding specific palette values in a container, if there is a special container palette applied to it. It demonstrates usage with `--headline-colour` usage in CardHeadline, which is overridden instead of having the three following values: - text.headline - text.cardHeadline - text.dynamoHeadline
mxdvl
force-pushed
the
darkmode/container-overrides
branch
from
d8595db to
7f8c960
Compare
abeddow91
approved these changes
Nov 14, 2023
|
|
||
| const paletteOverrides = { | ||
| '--headline-colour': isDynamo | ||
| ? text?.dynamoHeadline ?? decidePalette(format).text.dynamoHeadline |
There was a problem hiding this comment.
Is the plan still to remove decidePalette at some point?
cemms1
approved these changes
Nov 14, 2023
Comment on lines
34
to
38
| const paletteOverrides = { | ||
| '--headline-colour': isDynamo | ||
| ? text?.dynamoHeadline ?? decidePalette(format).text.dynamoHeadline | ||
| : text?.cardHeadline ?? decidePalette(format).text.cardHeadline, | ||
| } satisfies Partial<Record<ColourName, string>>; |
There was a problem hiding this comment.
I find this quite tricky to read. I wonder if there's another way we could do this? Maybe bring the logic out into its own function, or create a "fallback" type helper function.
There was a problem hiding this comment.
Thanks for flagging! Which part specifically is tricky to read?
A helper function can removed the need for the satisfies Partial<Record<ColourName, string>> but it cannot do away with the inherent complexity of how to pick the headline colour and its fallbacks.
Comment on lines
+43
to
+44
| css={[displayContents]} | ||
| style={paletteOverrides} |
There was a problem hiding this comment.
Why do we use both css and style? Is it because of this?
There was a problem hiding this comment.
And also because inline styles have the highest specificity. You can clearly see what the colour override is without even having to look at the resulting CSS declaration generated by Emotion.
Comment on lines
+555
to
+560
| declare namespace React { | ||
| interface CSSProperties { | ||
| // Allow custom properties to be passed to the style prop | ||
| [key: `--${string}`]: string | undefined; | ||
| } | ||
| } |
There was a problem hiding this comment.
I can see why it's here 🤔 but I thought we wanted to avoid using the global types file?
There was a problem hiding this comment.
I do not think this would work in any other file. It think it’s okay to have things that are truly global in there, such as the React namespace. Do you have an idea of where it could go otherwise?
--headline-colour is overriden at the CardHeadline level
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What does this change?
Create a wrapper component similar to
FormatBoundarythat allows to create container overrides for existing palette colours.Why?
This is a requirement as part of our move to the new palette and progressing with #9110
Screenshots
See Chromatic. There’s a demo of it in the new
CardHeadlinestory: