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(config): typography 및 fontWeight설정 #16

Merged
merged 2 commits into from
Jul 31, 2024
Merged

Conversation

qkrdmstlr3
Copy link
Member

1. 무슨 이유로 코드를 변경했나요?

typography와 fontWeight토큰을 추가합니다.

3. 관련 스크린샷을 첨부해주세요.

스크린샷 2024-07-30 오전 12 51 33

5. 추가 사항 / 코드 리뷰 받고 싶은 부분

typography는 text prefix와 함께, fontWeight는 font prefix와 함께 사용합니다

className="text-title1 font-bold"

@qkrdmstlr3 qkrdmstlr3 self-assigned this Jul 29, 2024
@qkrdmstlr3 qkrdmstlr3 requested review from eunbeann, Collection50 and woo-jk and removed request for eunbeann, woo-jk and Collection50 July 29, 2024 15:54
Copy link
Member

@Collection50 Collection50 left a comment

Choose a reason for hiding this comment

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

고생하셨습니다 !!

코드에 관해 코멘트 남겨드렸습니다 !!

Comment on lines +16 to +17
const fontWeightToken = fontWeightVariant.reduce((acc, token) => ({ ...acc, [token]: fontWeight[token] }), {});

Copy link
Member

Choose a reason for hiding this comment

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

이 부분과 아래 extend의 fontSize는 tailwind에서 제공하는 유틸리티와 동일한 것으로 보여지는데 맞을까요??

Copy link
Member Author

Choose a reason for hiding this comment

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

아 그러네요. fontSize는 기존 extend문에 추가하였고, fontWeight도 비슷한 위치에 넣어주었습니다. extend 대신 theme에 바로 넣는 것을 기대하셨을까요?

Copy link
Collaborator

@woo-jk woo-jk left a comment

Choose a reason for hiding this comment

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

LGTM👍

Comment on lines +37 to +39
body1: '24px',
body2: '22px',
label1: '20px',
Copy link
Collaborator

Choose a reason for hiding this comment

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

피그마를 보니 body1, body2, label1 토큰 같은 경우 Reading이라는 속성이 있는 것 같은데, 이 부분은 대응을 안해도 괜찮을까요?

Copy link
Member Author

Choose a reason for hiding this comment

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

사실 어떻게 이름을 지어야할지 몰라서.. 필요할 때 추가하려했습니다 ㅠ

@qkrdmstlr3 qkrdmstlr3 merged commit 35426de into main Jul 31, 2024
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants