Conversation
|
Thanks for the PR! 🎉 We've deployed an automatic preview for this PR - you can see your changes here:
Note The build needs to finish before your changes are deployed. |
|
|
||
| } | ||
|
|
||
| export function registerCustomSemanticVariableValue(name, lightValue, darkValue) { |
There was a problem hiding this comment.
This feels like a double-edged sword, but unless we define all colors inside core I see consumers needing custom colors(e.g. lumi)
There was a problem hiding this comment.
Yeah, this is a really slippery slope eh? If they can define there own colors, then it makes it really hard for us to make palette changes with confidence. Also, this requires that the consumer know the different color modes.
We've been having some discussion about the Lumi stuff. It still might be an issue, but I think the "AI colors" might get added to the semantic palette. There's also been some discussion of possibly adding a special button to core, given that AI is becoming more ubiquitous. But that's just the AI case. We also want to avoid multiple people each wanting some unique styles to highlight a new flashy feature, so there is also some discussion about generalizing that.
The Figma extract has a color for FACE background, but I did not include it because Jeff said it was somewhat experimental and we need to find a more general semantic for it. I think we should add this to the Confluence doc (as well as for d2l-collapsible-panel). I think there will be a color for it, it's just undefined atm.
There was a problem hiding this comment.
I still think we would need a way to programmatically set a custom variable. Not sure what we ended up deciding for color input but if we want to support users being able to set colors for both palettes we would need something like this helper
There was a problem hiding this comment.
I've made the helper internal, I would hope consumers can see red flags when trying to import it
There was a problem hiding this comment.
Since this is just for demo at the moment, what do you think about just adding the variables that we anticipate, knowing that the name will likely change, and keeping this helper in our back pocket until we need it in non-demo/test code? The colors (from Figma) are currently defined as:
// light
--d2l-theme-background-color-face: var(--d2l-color-gypsum);
// dark
--d2l-theme-background-color-face: #303335;I'd only excluded it earlier because we think it is going to be redefined, but it's really no different than the other variables we've added that we expect to change.
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
| } | ||
| :host([href]:not([_active]):hover) { | ||
| box-shadow: 0 2px 14px 1px rgba(0, 0, 0, 0.06); | ||
| box-shadow: var(--d2l-theme-shadow-attached); |
There was a problem hiding this comment.
I agree that "attached" is probably what we want here, at least based on how the shadows are currently defined.
We have subtle card shadow...
from: 0 4px 8px 0 rgba(0, 0, 0, 0.03)
to: 0 2px 4px 0 rgba(0, 0, 0, 0.03) (attached)
hover from: 0 4px 18px 2px rgba(0, 0, 0, 0.06)
to: 0 2px 12px 0 rgba(0, 0, 0, 0.15) (floating)
We have normal card shadow...
hover from: 0 2px 14px 1px rgba(0, 0, 0, 0.06)
to: 0 2px 12px 0 rgba(0, 0, 0, 0.15) (floating)
Do I have that right?
Those seem pretty similar, but the floating semantic doesn't seem right to me, and I'm not sure this is being considered in the shadow design revisions.
2 participants
GAUD-9457