-
Notifications
You must be signed in to change notification settings - Fork 1
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
feat: Handy Primitive Token 추가 #125
Conversation
3f1edc2
to
df8e6c9
Compare
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.
이 리뷰를 볼 쯤엔 2.0이 되어있겠군요
src/style/foundation/color/color.mdx
Outdated
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.
이거 semantic 토큰 제작하면서 제가 같이 진행할게여
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.
젠장 또 대상혁이야
@@ -0,0 +1,27 @@ | |||
// https://www.figma.com/design/gvwhMF6iNkuYKzxipKzkaG/Handy-v1-(demo)?node-id=636-131805&t=Wr7H8VzVaJXsyDQT-1 |
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에도 같이 첨부하면 좋겠네요!
다음부터는 PR에도 넣겠습니다
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.
안타깝게도 하나하나 쳤습니다..
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.
primitive spacing, primitive radius 같은 경우 바로 Sematic Token으로 만드는 것이 좋을 것 같아 Primitive Token으로 만들지 않았습니다.
이유도 같이 알려주시면 좋을 거 같아요!
(컬러값에 비해 단순해서 세밀한 구분이 필요하지 않아서 그런가요?)
color token은 primitive - semantic 구조로 정의하는데,
spacing token은 semantic으로 바로 정의한다면 일관성이 약간 깨지는 거 같아서
아직 잘 이해가 안돼요 😂
저도 동의합니다 |
업데이트 @2wndrhs 의 안구이슈로 제가 대신 진행하였습니다. 진행사항은 아래와 같습니다
YDSTheme 필드 타입 관련우선 변경된 필드는 아래와 같습니다. export type YDSTheme = {
primitive: {
color: PrimitiveColorPalette;
spacing: Spacings;
};
// TOOD: semantic pr 에서 진행합니다.
// semantic: {
// color: SemanticColorPalette;
// spacing: SemanticSpacings;
// };
typo: Typos;
}; theme 내부에서 primitive와 semantic을 확실하게 구분하는게 좋겠다고 판단하여 저렇게 작성했는데요, 이런 의견이 나올수도 있어서 작성했습니다. export type YDSTheme = {
color: PrimitiveColorPalette & SemanticColorPalette;
spacing: Spacings & SemanticSpacings;
typo: Typos;
};
를 고민해봤는데요,
의 흐름으로 판단했습니다.
|
ㅋㅋㅋㅋㅋ 그의 빠른 안구 회복을 기대하며,,,,, 고생하셨습니다!!! 🎉 |
spacing token을 primitive로 정의하면 아래와 같이 프로퍼티의 키를 숫자로 정의해야 하는데 const spacing: Record<SpacingType, number> = {
'8': 8,
'10': 10,
'12': 12,
'14': 14,
'16': 16,
}; 이게 어색하게 느껴져서 아래처럼 semantic 토큰으로 바로 정의하려 했습니다! export const spacing: Record<SpacingSize, string> = {
xs: '8px',
s: '10px',
m: '12px',
l: '14px',
xl: '16px',
} as const; 그렇지만 말씀하신 것처럼 일관성이 깨지는 문제도 있고 이번에 디자인 토큰도 number와 radius/spacing으로 primitive와 semantic간 구분이 명확해졌으니 지금처럼 구분해서 관리하는 것이 좋을 것 같네요! |
1️⃣ 어떤 작업을 했나요? (Summary)
기존 코드에 영향을 미치지 않는 변경사항
기존 YDS에서 사용하던
baseColor
들을 삭제하고 Handy Design System에서 새로 정의된primitiveColor
를 추가하였습니다.2️⃣ 알아두시면 좋아요!
3️⃣ 추후 작업
4️⃣ 체크리스트 (Checklist)
main
브랜치의 최신 코드를pull
받았나요?