Recolouring follow-up #2742
Recolouring follow-up #2742
Conversation
🦋 Changeset detectedLatest commit: 16be228 The changes in this PR will be included in the next version bump. This PR includes changesets to release 95 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Is this PR ready for review? |
@CarlosCortizasCT Thanks for checking, yes, it is |
| @@ -45,6 +45,7 @@ choiceGroupsByTheme: | |||
| color-primary-85: 'hsl(175, 70%, 85%)' | |||
| color-primary-90: 'hsl(175, 70%, 90%)' | |||
| color-primary-95: 'hsl(175, 90%, 95%)' | |||
| color-primary-98: 'hsl(244, 100%, 99%)' # design token copied from recolouring theme for export | |||
There was a problem hiding this comment.
I know maybe this token won't be used, but I think its value should be aligned with the other color primary tokens:
color-primary-95: 'hsl(175, 90%, 99%)'
There was a problem hiding this comment.
Sorry, I don't fully understand this one. Is the idea to create a placeholder token value solely due to limitations in our tooling? I believe I would need clarification from Filip on this as I'm not familiar with the pattern.
Upon further reflection, it might be beneficial to use a placeholder value like unused or something similar. What do you think?
There was a problem hiding this comment.
Since default is the base theme, I think it should have all the available tokens.
I understand it might not needed in this case, but I think it's a good idea from the maintainability point of view.
The way I see it, I don't see the benefit in including a placeholder value but just think about what the name of the token represent and have its value in the default theme. That's why I suggest to use the proposal I shared in my previous comment.
We can have a call to discuss this further if you want to 👍
There was a problem hiding this comment.
Thanks 🙏 Still, I don't fully get the full picture and what is the proposed value for this choice, so I'd rather schedule a call to discuss it
packages/components/icons/src/leading-icon/leading-icon.styles.ts
Outdated
Show resolved
Hide resolved
packages/components/inputs/radio-input/src/radio-option.styles.ts
Outdated
Show resolved
Hide resolved
| @@ -41,14 +41,14 @@ const getButtonStyles = (isDisabled: boolean) => { | |||
| css` | |||
| background-color: ${designTokens.colorSurface}; | |||
| box-shadow: ${designTokens.shadow0}; | |||
| border: ${`1px solid ${designTokens.colorNeutral}`}; | |||
| border: 1px solid ${designTokens.borderColorForButtonAsSecondary}; | |||
There was a problem hiding this comment.
As my previous comment, I think we should avoid reusing component tokens among different components.
We are already using the dropdown token names below, so I think it would be better to do the same here.
There was a problem hiding this comment.
@CarlosCortizasCT Thank you for the review 🙏 I covered token renaming in 2a0f279. The PR is ready for another review round
| @@ -45,6 +45,7 @@ choiceGroupsByTheme: | |||
| color-primary-85: 'hsl(175, 70%, 85%)' | |||
| color-primary-90: 'hsl(175, 70%, 90%)' | |||
| color-primary-95: 'hsl(175, 90%, 95%)' | |||
| color-primary-98: 'hsl(244, 100%, 99%)' # design token copied from recolouring theme for export | |||
There was a problem hiding this comment.
Sorry, I don't fully understand this one. Is the idea to create a placeholder token value solely due to limitations in our tooling? I believe I would need clarification from Filip on this as I'm not familiar with the pattern.
Upon further reflection, it might be beneficial to use a placeholder value like unused or something similar. What do you think?
kark
force-pushed
the
SHIELD-1134-recolouring-follow-up
branch
from
b432d63
to
e03508c
Compare
Regarding this one, do you prefer to not change it? |
Updated in 6131eb2 |
Summary
Figma file