Add new tokens for border radius #4401
Open
+687
−131
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
RobinCsl
commented
Jun 28, 2024
| "value": "{foundation.border-radius.normal}", | ||
| "deprecated": true, | ||
| "deprecated-replace": "{global.border-radius.100}", | ||
| "deprecated-version": "16.0.0" |
There was a problem hiding this comment.
Not sure if that's the expected version that's supposed to be put here.
RobinCsl
commented
Jun 28, 2024
RobinCsl
commented
Jun 28, 2024
|
Storybook staging is available at https://kiwicom-orbit-rcsl-add-new-tokens.surge.sh |
|
Size Change: +323 B (+0.07%) Total Size: 446 kB
ℹ️ View Unchanged
|
Deploying orbit with
|
| Latest commit: |
d149983
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://4638e35c.orbit.pages.dev |
| Branch Preview URL: | https://rcsl-add-new-tokens.orbit.pages.dev |
RobinCsl
force-pushed
the
rcsl/add-new-tokens
branch
2 times, most recently
from
dfd0617
to
3c223df
Compare
sarkaaa
force-pushed
the
rcsl/add-new-tokens
branch
from
3c223df
to
6c7d0bc
Compare
DSil
force-pushed
the
rcsl/add-new-tokens
branch
2 times, most recently
from
3c223df
to
893c867
Compare
sarkaaa
force-pushed
the
rcsl/add-new-tokens
branch
8 times, most recently
from
d0692fb
to
96cd3df
Compare
|
Hello, I updated border radius values, updated visual screenshots and snapshots for the tests, but still getting failed unit test - https://github.com/kiwicom/orbit/actions/runs/10038370965/job/27740127403?pr=4401 I guess it's caused that |
|
Hmm I am not sure I understand your problem, and it seems it relies in some wrong assumptions / statements:
This is not true! The tokens did not change values. New tokens were added to replace old ones that will be removed soon (hence the deprecation). The old tokens must not change their values. Otherwise, it is a breaking change. More than that, the test is correct. If the token did not change its value (as expected), it still computes to 3px. Now… how is the snapshot expecting 4px? How did this change happen? |
sarkaaa
force-pushed
the
rcsl/add-new-tokens
branch
from
96cd3df
to
5ec5b41
Compare
I described it in a confusing way, but I can't explain myself how the received value can be The values weren't changed for old names, but:
That was for testing purposes, sorry. 🙈 The unit test is failing without these updated snapshots anyway. |
That is expected. It is a visual change made by the designers that we adopt.
Well, there were two failing and now there is only one 😄 The tokens one was failing and it should not. Now, the one failing is on Box. If we look at the definition of Box, we see that it expects the The Box component is a special component, very tied to the tokens. So in my opinion we should:
This way we avoid the breaking change for now and we can remove the prop values when we remove the tokens (preventing inconsistencies) |
|
Thank you for your response. I get the reasons now. This statement ⬇️ is not clear to me:
Which props and where (file) do you mean? Can you provide more details, please? Btw, I added new types to |
sarkaaa
reviewed
Jul 23, 2024
packages/orbit-design-tokens/src/dictionary/definitions/global/borderRadius.json
Show resolved
Hide resolved
sarkaaa
force-pushed
the
rcsl/add-new-tokens
branch
from
7bc3c19
to
7c64d0d
Compare
sarkaaa
changed the title
|
Jira ticket: https://kiwicom.atlassian.net/browse/FEPLT-2050 |
sarkaaa
force-pushed
the
rcsl/add-new-tokens
branch
2 times, most recently
from
c150270
to
f370091
Compare
DSil
reviewed
Jul 24, 2024
docs/src/documentation/05-development/04-migration-guides/01-v16.mdx
Outdated
Show resolved
Hide resolved
sarkaaa
force-pushed
the
rcsl/add-new-tokens
branch
2 times, most recently
from
07a91e6
to
f09b84f
Compare
DSil
force-pushed
the
rcsl/add-new-tokens
branch
3 times, most recently
from
4cd77ed
to
54208ca
Compare
sarkaaa
reviewed
Jul 26, 2024
| @@ -32,8 +32,8 @@ const StyledTab = styled.button<{ active: boolean }>` | |||
| ${({ theme, active }) => css` | |||
| padding: 14px ${theme.orbit.spaceMedium}; | |||
| margin-bottom: -1px; | |||
| border-top-left-radius: ${theme.orbit.borderRadiusLarge}; | |||
| border-top-right-radius: ${theme.orbit.borderRadiusLarge}; | |||
| border-top-left-radius: ${theme.orbit.borderRadius150}; | |||
There was a problem hiding this comment.
We want to include the new tokens in styled-components as well, right?
@DSil
There was a problem hiding this comment.
We have to. Styled components use the tokens directly. If we remove the tokens, styled components can't get the values from it. That is why we need to update the tokens there as well.
sarkaaa
reviewed
Jul 26, 2024
|
|
||
| A codemod was made avaialble to help with the migration. It should target all the tokens and tailwind classes that were updated and correctly migrate them. | ||
|
|
||
| Please ensure the changes are expected and there is no change missing. Please note that the codemod **does not** cover the Box component changes. |
There was a problem hiding this comment.
The extended guidelines look very nice! :)
sarkaaa
reviewed
Jul 26, 2024
| "rounded-large bg-ink-dark p-xs relative w-full overflow-hidden will-change-transform", | ||
| "lm:max-w-[360px] lm:w-auto lm:p-sm [&_svg]:min-h-icon-medium", | ||
| "rounded-150 bg-ink-dark p-xs relative w-full overflow-hidden will-change-transform", | ||
| "lm:max-w-modal-extra-small lm:w-auto lm:p-sm [&_svg]:min-h-icon-medium", |
There was a problem hiding this comment.
It looks it's renamed again. 😢 Can you take a look at it, pls? I don't want to make a mess during your changes. @DSil
sarkaaa
reviewed
Jul 26, 2024
DSil
force-pushed
the
rcsl/add-new-tokens
branch
2 times, most recently
from
767ecdb
to
855acae
Compare
| "value": "{foundation.border-radius.small}", | ||
| "deprecated": true, | ||
| "deprecated-replace": "{global.border-radius.50}", | ||
| "deprecated-version": "9.0.0" |
There was a problem hiding this comment.
Why version 9.0.0? Does this correspond to the orbit-design-tokens package version?
There was a problem hiding this comment.
This was supposed to be a reference for the next version of orbit-design-tokens version in which we would remove this deprecated token. However, we're not really following it strictly, it just needs a value 😕
| { from: /rounded(-[a-z]+)?-small/, to: "rounded$1-50" }, | ||
| { from: /rounded(-[a-z]+)?-normal/, to: "rounded$1-100" }, | ||
| { from: /rounded(-[a-z]+)?-large/, to: "rounded$1-150" }, | ||
| { from: /rounded(-[a-z]+)?-circle/, to: "rounded$1-full" }, |
There was a problem hiding this comment.
Isn't this a visual breaking change? Is this transforming 50% to 9999px? Or am I missing something?
There was a problem hiding this comment.
It should be visually the same (PR here, that include links for dicussions). And -circle is deprecated for a while now.
| { from: "borderRadiusSmall", to: "borderRadius50" }, | ||
| { from: "borderRadiusNormal", to: "borderRadius100" }, | ||
| { from: "borderRadiusLarge", to: "borderRadius150" }, | ||
| { from: "borderRadiusCircle", to: "borderRadiusFull" }, |
| @@ -0,0 +1,71 @@ | |||
| /* eslint-disable no-console */ | |||
There was a problem hiding this comment.
This commit feat: create codemod for new tokens and tailwind classes is not scoped but is a feat. Will it show in the changelog? 🤔
There was a problem hiding this comment.
It will display on the orbit-components changelog. The scope is only used to add a bold context on the changelog.
On the current changelog, version 15.8.0 has a feature with no scope (the last one).
| | `"normal"` – **deprecated (use `"100"`)** | | ||
| | `"large"` – **deprecated (use `"150"`)** | | ||
| | `"circle"` – **deprecated (use `"full"`)** | | ||
| | `"full"` | |
There was a problem hiding this comment.
Is none missing?
Should we move full at the bottom to have them sorted?
There was a problem hiding this comment.
No, Box does not support none. Since borderRadius is an optional prop, having it undefined should be the same as having it none 🤔 Unless we want to specifically allow the inclusion of a prop value that forces a class to define border-radius: 0… I am not against it… WDYT?
| @@ -52,14 +52,14 @@ const TypeIcon = ({ type, active, mobile }: Props) => { | |||
| return ( | |||
| <div | |||
| className={cx( | |||
| "z-default relative flex h-[20px] items-center justify-center", | |||
| "z-default h-icon-medium relative flex items-center justify-center", | |||
There was a problem hiding this comment.
Here I guess it makes sense, as the component is TypeIcon.
There was a problem hiding this comment.
Yes! This one I noticed and left on purpose. The other ones I completely missed 🫣
|
|
||
| ```jsx | ||
| <Box borderRadius="small">small borderRadius</Box> | ||
| <Box borderRadius="normal">normal borderRadius</Box> |
There was a problem hiding this comment.
Related to this, I think this one is missing.
There was a problem hiding this comment.
You mean replacing the value of the prop in that story? Yes, I will change
| **Now:** | ||
|
|
||
| ```jsx | ||
| <Box borderRadius="50">"50" borderRadius</Box> |
There was a problem hiding this comment.
Are the quotes in the children's values intended? As they are not present in the Before snippet.
There was a problem hiding this comment.
Yeah, I don't know what happened 😅 I don't know which one I prefer. But since they're strings I'll just add the quotes on the Before snippets.
|
|
||
| ## Codemod | ||
|
|
||
| A codemod was made avaialble to help with the migration. It should target all the tokens and tailwind classes that were updated and correctly migrate them. |
There was a problem hiding this comment.
s/avaialble/available
| Please ensure the changes are expected and there is no change missing. Please note that the codemod **does not** cover the Box component changes. | ||
|
|
||
| ```bash | ||
| node .../transforms/tokens-v16.mjs |
There was a problem hiding this comment.
I'm not sure if the 3 dots can work here. Is this the final command?
There was a problem hiding this comment.
No, this part of the migration guide is WIP, please disregard it for this PR
The change ensures we're correctly transforming the dot notation to access object properties when numbers are used as keys: 'foundations.borderRadius.50' is incorrect JavaScript; 'foundations.borderRadius["50"]' is valid JavaScript.
New tokens: none, 50, 100, 150, 300, 400 Deprecated tokens: small, normal, large
New classes: rounded-50, rounded-100, rounded-150, rounded-300, rounded-400 Deprecated: rounded-small, rounded-normal, rounded-large
New possible values: "none", "full", "50", "100", "150", "300", "400" Deprecated: "small", "normal", "large", "circle"
DSil
force-pushed
the
rcsl/add-new-tokens
branch
from
a897565
to
d149983
Compare
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.
FEPLT-2050
What remains to be done:
rounded-50classes not being part of thepackages/orbit-components/lib/tailwind.cssgenerated file for some reason (might be a step I missed);Commits:
docs: add changelog draft for v16
feat: update Tailwind classes for new border radius tokens
through the new transform.
Changes generated by running: 'node ../transforms/tokens-v16.mjs' from
the 'src' folder in packages/orbit-components.
BREAKING CHANGE: All the classes using normal border radius will now see
a 1px larger border radius through the Orbit Design Token update and
mapping between old tokens and new.
feat(tokens): add new tokens for border-radius
Slack: https://skypicker.slack.com/archives/C7T7QG7M5/p1719493641111859
chore(tokens): allow numbers as keys in tokens
The change ensures we're correctly transforming the dot notation to access
object properties when numbers are used as keys:
'foundations.borderRadius.50' is incorrect JavaScript;
'foundations.borderRadius["50"]' is valid JavaScript.
This Pull Request meets the following criteria:
For new components:
d.tsfiles and are exported inindex.d.ts