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. |
| --d2l-sem-shadow-inset-blur: 0; | ||
| --d2l-sem-shadow-inset-spread: 0; | ||
| --d2l-sem-shadow-inset: inset var(--d2l-shadow-inset-offset-x) var(--d2l-shadow-inset-offset-y) var(--d2l-shadow-inset-blur) var(--d2l-shadow-inset-spread) var(--d2l-shadow-inset-color); | ||
| --d2l-background-color-base: #ffffff; |
There was a problem hiding this comment.
I'll admit that it does feel a bit naked now, but I think I still prefer it to sem.
What about something like -theme-? So --d2l-theme-background-color-base.
There was a problem hiding this comment.
Definitely an option I think.
There was a problem hiding this comment.
Other ones I'd thought of were --d2l-core-* or --d2l-ui-*. Don't necessarily love them, but they're short and I like them more than sem.
There was a problem hiding this comment.
d2l-mode-* is also an option if four letters is fine and we're set on different color/spacing "modes" 🤷♀️
| --d2l-sem-shadow-inset: inset var(--d2l-shadow-inset-offset-x) var(--d2l-shadow-inset-offset-y) var(--d2l-shadow-inset-blur) var(--d2l-shadow-inset-spread) var(--d2l-shadow-inset-color); | ||
| --d2l-background-color-base: #ffffff; | ||
| --d2l-background-color-elevated: var(--d2l-background-color-base); | ||
| --d2l-background-color-floating: var(--d2l-background-color-base); |
There was a problem hiding this comment.
We could also tighten up the background-color to just be bg-color and fg-color, which I've noticed GH does.
There was a problem hiding this comment.
Yeah I wouldn't mind that - in effort to keep them a bit shorter, more readable, etc.
With that, would --d2l-text-color-static-faint become --d2l-text-fg-color-static-faint? And --d2l-icon-color-faint become --d2l-icon-fg-color-faint? There are status colors too but they are different - they can be applied as either border and backgrounds.
There was a problem hiding this comment.
It gets a little murky yeah. bg-color seems obvious. We could say fg-color applies to text so yeah that would mean we could drop the -text- bit. If an icon color and the text colour are different (which they are in light mode anyway), then we could just keep d2l-icon-color and not worry about putting fg in it.
There was a problem hiding this comment.
So three options I think:
- Keep it as it is, with the option of shortening
backgroundtobg. - Treat
fg(foreground) equivalent totext, sod2l-text-color-basebecomesd2l-fg-color-base(dropping thetext). But icons are different so they'd still bed2l-icon-color. - Put
fgin front of all "foreground" things, which makes itd2l-text-fg-colorandd2l-icon-fg-color. It might be a little weird but in theory it would allow us to haved2l-text-bg-colorif we ever wanted that.
There was a problem hiding this comment.
I nudged @geurts since there an effort to keep the language in Figma fairly aligned with the vocabulary in code. Removing text here but not in Figma would be a more noteworthy difference.
There was a problem hiding this comment.
I didn't really understand 3 ... so you're saying we could add "fg" or "bg" after "text" and "icon", so that they have both "fg" and "bg" variables (though initially we don't have any use for "bg" variables for these)?
There was a problem hiding this comment.
One solution we didn't discuss, but which is constantly rattling around in my head, is to update icons to use the same "standard" colour as text. This would bring them closer together so we could just have a "fg" top level category that addresses both icons and text. I remember making them different was a tough call at the time, and I could have gone either way.
Example — before:
After:
There was a problem hiding this comment.
Yeah exactly, and Dave's example of where we might have a background for text would be selected. But yeah, if that discrepancy between text & icon colours went away, then it could just be fg and be used by both.
There was a problem hiding this comment.
Although if we did that, consider what it might mean if we need them to be distinct later.
4 participants
GAUD-9068
This removes
-sem-from the semantic variables.I don't feel strongly about it. It sounds like Jeff might lean towards keeping it. If devs really prefer not having it, we'll remove it before applying it to a lot of components.