Repository navigation
Design Tokens: add new color and font-family CSS Custom Properties - #4749
RasmusKjeldgaard wants to merge 8 commits into
Conversation
5433339 to
18bfb9c
Compare
18bfb9c to
eed4efd
Compare
ef0ccb3 to
edfa3d2
Compare
edfa3d2 to
da6b5bf
Compare
da6b5bf to
3518963
Compare
cfed8bc to
53e5408
Compare
ae874b3 to
918c4e5
Compare
918c4e5 to
8890e0b
Compare
8890e0b to
93ad535
Compare
93ad535 to
f37156c
Compare
3a113b9 to
60fdf73
Compare
Loujuu
left a comment
There was a problem hiding this comment.
Nice with Figma generate files. I do think they need a closer look. Seems like there is currently many doublications and some improvements to the structure.
| */ | ||
|
|
||
| :root { | ||
| --kirby-system-color-light-grey-25: #ffffff; |
There was a problem hiding this comment.
This naming is a bit misleading. #ffffff is white. kirby-system-color-light-grey-25. I do also wonder how a greyscale can represent a pure white?
There was a problem hiding this comment.
I think it is not uncommon to have a palette that includes black/white on opposite ends.
https://m3.material.io/styles/color/system/how-the-system-works
But the naming and increments could possibly be improved, we'll have to discuss with designers.
| */ | ||
|
|
||
| :root { | ||
| --kirby-system-color-light-grey-25: #ffffff; |
There was a problem hiding this comment.
Could it be confusing to use light-grey and dark-grey. light-grey-975 will not be light. And dark-grey-25 will not be dark. I wonder if we actually need 25 shades?
| --kirby-color-fill-danger-quiet-hover: var(--kirby-system-color-red-100); | ||
| --kirby-color-fill-danger-quiet-active: var(--kirby-system-color-red-50); | ||
| --kirby-color-content-base-loud: var(--kirby-system-color-dark-grey-975); | ||
| --kirby-color-content-base-loud-hover: var(--kirby-system-color-dark-grey-975); |
There was a problem hiding this comment.
Is it intentional that var(--kirby-system-color-dark-grey-975); is used for different states: default/hover/active?
| --kirby-color-content-base-hover: var(--kirby-system-color-dark-grey-975); | ||
| --kirby-color-content-base-active: var(--kirby-system-color-dark-grey-975); | ||
| --kirby-color-content-base-quiet: var(--kirby-system-color-dark-grey-500); | ||
| --kirby-color-content-base-quiet-hover: var(--kirby-system-color-dark-grey-500); |
There was a problem hiding this comment.
Same issue: var(--kirby-system-color-dark-grey-500); used for different states default/hover/active.
| --kirby-color-content-raised-loud: var(--kirby-system-color-dark-grey-975); | ||
| --kirby-color-content-raised-loud-hover: var(--kirby-system-color-dark-grey-975); | ||
| --kirby-color-content-raised-loud-active: var(--kirby-system-color-dark-grey-975); | ||
| --kirby-color-content-raised: var(--kirby-system-color-dark-grey-975); |
There was a problem hiding this comment.
Same issue regarding using same color for all states for --kirby-color-content-raised.
| --kirby-toggle-color-fill-engaged: var(--kirby-system-color-green-600); | ||
| --kirby-toggle-color-fill-engaged-hover: var(--kirby-system-color-green-700); | ||
| --kirby-toggle-color-fill-engaged-active: var(--kirby-system-color-green-800); | ||
| --kirby-toggle-color-content: var(--kirby-system-color-white-00); |
There was a problem hiding this comment.
default/hover/active have same: (--kirby-system-color-white-00). If no states needed delete hover and active
| --kirby-toggle-color-fill-engaged: var(--kirby-system-color-green-400); | ||
| --kirby-toggle-color-fill-engaged-hover: var(--kirby-system-color-green-300); | ||
| --kirby-toggle-color-fill-engaged-active: var(--kirby-system-color-green-200); | ||
| --kirby-toggle-color-content: var(--kirby-system-color-dark-grey-975); |
There was a problem hiding this comment.
default/hover/active have same color. If no states needed delete hover and active
| --kirby-color-content-danger-loud-hover: var(--kirby-system-color-white-00); | ||
| --kirby-color-content-danger-loud-active: var(--kirby-system-color-white-00); | ||
| --kirby-color-content-danger: var(--kirby-system-color-dark-grey-975); | ||
| --kirby-color-content-danger-hover: var(--kirby-system-color-dark-grey-975); |
There was a problem hiding this comment.
In genrel, check if state colors are needed or delete. default/hover/active have same. If no states needed delete hover and active
| --kirby-color-border-raised-quiet: var(--kirby-system-color-light-grey-700); | ||
| --kirby-color-border-raised-quiet-hover: var(--kirby-system-color-light-grey-700); | ||
| --kirby-color-border-raised-quiet-active: var(--kirby-system-color-light-grey-700); | ||
| --kirby-color-border-raised-silent: var(--kirby-system-color-light-grey-600); |
There was a problem hiding this comment.
The same token three times and with different values. Seems to be a bug?
Please go though the tokens to see if this is a single mistake.
Also states with same value.
I will not go though every token :-)
(137-139)
--kirby-color-border-raised-silent: var(--kirby-system-color-light-grey-600);
--kirby-color-border-raised-silent-hover: var(--kirby-system-color-light-grey-600);
--kirby-color-border-raised-silent-active: var(--kirby-system-color-light-grey-600);
(314-316)
--kirby-color-border-raised-silent: var(--kirby-system-color-light-grey-600);
--kirby-color-border-raised-silent-hover: var(--kirby-system-color-light-grey-600);
--kirby-color-border-raised-silent-active: var(--kirby-system-color-light-grey-600);
(491-493)
--kirby-color-border-raised-silent: var(--kirby-brand-color-dark-blue-700);
--kirby-color-border-raised-silent-hover: var(--kirby-brand-color-dark-blue-600);
--kirby-color-border-raised-silent-active: var(--kirby-brand-color-dark-blue-500);
| @@ -1,4 +1,9 @@ | |||
| @use 'utils'; | |||
|
|
|||
| @use 'themes/primitives'; | |||
There was a problem hiding this comment.
Could we discuss the theming to better understand this concept of dividing the tokens.
b1ba96c to
e463f41
Compare
e463f41 to
4a03669
Compare
4a03669 to
61183f2
Compare
This is the output from running the new Design Token pipeline on the current figma exports. Final details still missing around naming and existing specs (spacing, font-weight etc) in figma, so the files are not yet exported or used.
Ignore unused or not-ready-for-dev variables
61183f2 to
b32e850
Compare
Which issue does this PR close?
This PR adds token output from #4745
What is the new behavior?
New set of Design tokens for the re-branding are added to
:rootelement via the core package's global styles, and made available under specific surface selectors in preparation for using the tokens in all designsystem and extension components.This initial addition is mostly about color and font family, alongside the well-defined spacings etc. Any variables that are not yet fully refined are ignored for now, to be rolled out incrementally when needed.
Does this PR introduce a breaking change?
Are there any additional context?
Checklist:
The following tasks should be carried out in sequence in order to follow the process of contributing correctly.
Reminders
Review
When the pull request has been approved it will be merged to
developby Team Kirby.Stack created with GitHub Stacks CLI • Give Feedback 💬