-
Notifications
You must be signed in to change notification settings - Fork 48
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Convert legacy color tokens to alpha color tokens at runtime #2588
Conversation
🦋 Changeset detectedLatest commit: f3170c6 The changes in this PR will be included in the next version bump. This PR includes changesets to release 5 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 |
Chromatic Report🚀 Congratulations! Your build was successful! |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## alpha #2588 +/- ##
==========================================
+ Coverage 81.86% 81.91% +0.05%
==========================================
Files 145 145
Lines 2889 2897 +8
Branches 920 919 -1
==========================================
+ Hits 2365 2373 +8
Misses 494 494
Partials 30 30 ☔ View full report in Codecov by Sentry. |
…lor token conversion
89237e9
to
7875415
Compare
eba6a2e
to
bcbc99f
Compare
return alphaColorTokenCssVar( | ||
legacyToAlphaColorTokenMap[ | ||
colorToken as keyof typeof legacyToAlphaColorTokenMap | ||
] ?? colorToken |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
legacy -> alpha color map 이 없으면 alpha prefix 를 붙이면 안되지 않나요?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
기본적으로 1:1 대응을 하되, 이름이 다르거나 하여 대응이 안되는 경우(e.g. bgtxt-...)에 대해서 마이그레이션 테이블이 있는 거로 이해했어요.
"bg": { | ||
"black": { | ||
"darkest": { | ||
"value": "{black.60}", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이건 후속 PR에서 체크해보고 전반적으로 업데이트해볼게요. 색상값이 또 변경될 여지가 있다고 해서 어느정도 여유를 두고 진행해도 괜찮을 거 같아요
"value": "#373b5666", | ||
"type": "color" | ||
} | ||
"blue": { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Self Checklist
Related Issue
Summary
알파 컬러 마이그레이션 중 일부입니다.
ColorProps(
color
)를 사용하는 레거시 컴포넌트에서 레거시 시맨틱 컬러를 알파 컬러로 런타임 변환합니다.Details
커밋별로 확인해주세요
ColorProps
,elevation
유틸 클래스에서 알파 컬러를 지원 & 대체하도록 변경합니다.color
prefix가 불필요하다고 판단하여 제거했습니다. 피그마에선 functional만 color prefix가 있고, semantic은 없는 등 레거시 시맨틱 컬러와 동일하게 굳이 추가할 필요가 없겠다고 생각하여 제거했습니다.이 PR 이후에 아래와 같은 작업이 필요합니다.
Breaking change? (Yes/No)
Yes
알파 컬러로 변환된 일부 색상의 값이 변경됩니다. (Chromatic UI Review 확인)
References
초기에 토큰의 이름을 동일하게 두고, CSS layer를
tokens
,alpha-tokens
두개로 분리하고 알파 토큰에 더 우선순위를 두어 맵핑이 필요한 요소 말고는 최대한 변경이 적게 동일한 CSS variable 이름을 유지하는 방향을 생각했습니다(alpha-*** 가 붙지 않음). 테스트해보니 레거시 시맨틱 네임이 CSS layer의 우선순위로 인해 팔레트(e.g.blue-200
) 값을 참조할 때 우선순위로 인해 tokens 레이어가 아닌 alpha-tokens 레이어를 참조하는 문제가 있었습니다. 레거시 컬러를 사용했을 땐 레거시 컬러대로 잘 보여져야하는게 올바른 동작이라고 판단해서 이 방식은 기각했습니다.