Skip to content
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

Merged
merged 19 commits into from
Jul 21, 2024
Merged

Conversation

2wndrhs
Copy link
Member

@2wndrhs 2wndrhs commented Jul 11, 2024

1️⃣ 어떤 작업을 했나요? (Summary)

기존 코드에 영향을 미치지 않는 변경사항

기존 YDS에서 사용하던 baseColor들을 삭제하고 Handy Design System에서 새로 정의된 primitiveColor를 추가하였습니다.

image

2️⃣ 알아두시면 좋아요!

  • primitive spacing, primitive radius 같은 경우 바로 Sematic Token으로 만드는 것이 좋을 것 같아 Primitive Token으로 만들지 않았습니다.

3️⃣ 추후 작업

  • PR 리뷰 기반 수정

4️⃣ 체크리스트 (Checklist)

  • main 브랜치의 최신 코드를 pull 받았나요?

@2wndrhs 2wndrhs added the feat label Jul 11, 2024
@2wndrhs 2wndrhs self-assigned this Jul 11, 2024
@2wndrhs 2wndrhs force-pushed the feat/#122-handy-primitive-token branch from 3f1edc2 to df8e6c9 Compare July 12, 2024 08:20
Copy link
Collaborator

@nijuy nijuy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 리뷰를 볼 쯤엔 2.0이 되어있겠군요

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

문서 수정도 같이 해주시면



Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이거 semantic 토큰 제작하면서 제가 같이 진행할게여

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

얏호

Copy link
Member Author

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
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

피그마 찾아 들어가기 귀찮았는데 편하네요 굿.
(나중에 주석 지우기 깜빡할 수 있으니!!
이런 링크는 깃허브 코멘트로 달아주시는 것도 조을 거 같아요)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

말씀하신대로 PR에도 같이 첨부하면 좋겠네요!
다음부터는 PR에도 넣겠습니다

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

혹시 이거 하나하나 치셨나요
이런 일이 생겼을 때 손가락 덜 아프려면 꿀팁 같은거,, 있나 궁금해가지고,,,,,,

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

안타깝게도 하나하나 쳤습니다..

Copy link
Collaborator

@nijuy nijuy left a 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으로 바로 정의한다면 일관성이 약간 깨지는 거 같아서
아직 잘 이해가 안돼요 😂

@fecapark
Copy link
Contributor

primitive spacing, primitive radius 같은 경우 바로 Sematic Token으로 만드는 것이 좋을 것 같아 Primitive Token으로 만들지 않았습니다.

이유도 같이 알려주시면 좋을 거 같아요! (컬러값에 비해 단순해서 세밀한 구분이 필요하지 않아서 그런가요?)

color token은 primitive - semantic 구조로 정의하는데, spacing token은 semantic으로 바로 정의한다면 일관성이 약간 깨지는 거 같아서 아직 잘 이해가 안돼요 😂

저도 동의합니다

@fecapark
Copy link
Contributor

fecapark commented Jul 21, 2024

업데이트

@2wndrhs 의 안구이슈로 제가 대신 진행하였습니다.

진행사항은 아래와 같습니다

  • primitive에 spacing 토큰 구현
  • 라이트/다크모드 감지 로직 삭제 ( Handy에서는 구분하지 않기 때문 )
  • YDSTheme의 필드 타입을 변경하였습니다만....

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;
};

theme에서 primitive와 semantic을 구분해야하냐?

를 고민해봤는데요,

  1. 그러면 primitive와 semantic은 무엇이 다른가?

  2. primitive는 semantic을 만드는데 사용되고, semantic을 이용하여 컴포넌트와 레이아웃을 구성한다.

  3. 그러면 primitive는 배포할 필요가 없는가?

  4. primitive는 잘 사용되지는 않지만, 일반적으로 대부분의 디자인 시스템에서 관리와 확장성을 위해 보통 함께 배포한다. (YDS에서도 함께 배포했었음)

  5. => primitive 를 함께 배포하되, 실제 사용하는 semantic과 분리하자

의 흐름으로 판단했습니다.


  • 빌드 잘 돌아가고, spacing 및 color 잘 적용되는거 확인했습니다

@fecapark fecapark merged commit 17bdb68 into develop Jul 21, 2024
@fecapark fecapark deleted the feat/#122-handy-primitive-token branch July 21, 2024 07:29
@nijuy
Copy link
Collaborator

nijuy commented Jul 21, 2024

ㅋㅋㅋㅋㅋ 그의 빠른 안구 회복을 기대하며,,,,, 고생하셨습니다!!! 🎉
기존 YDSTheme 타입을 유지하려면 spacing을 추가할 때도 spacing/PrimitiveSpacing으로 나눠야 할텐데
이거보단 페카 작업 내용처럼 한번 쪼개는 게(?) 보기 편하네요 👍

@2wndrhs
Copy link
Member Author

2wndrhs commented Jul 22, 2024

primitive spacing, primitive radius 같은 경우 바로 Sematic Token으로 만드는 것이 좋을 것 같아 Primitive Token으로 만들지 않았습니다.

이유도 같이 알려주시면 좋을 거 같아요! (컬러값에 비해 단순해서 세밀한 구분이 필요하지 않아서 그런가요?)

color token은 primitive - semantic 구조로 정의하는데, spacing token은 semantic으로 바로 정의한다면 일관성이 약간 깨지는 거 같아서 아직 잘 이해가 안돼요 😂

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간 구분이 명확해졌으니 지금처럼 구분해서 관리하는 것이 좋을 것 같네요!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: Handy Primitive Token 추가
3 participants