-
Notifications
You must be signed in to change notification settings - Fork 4
refactor(typography): apply vanilla-extract #162
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
base: release/v1
Are you sure you want to change the base?
Conversation
- package 설정 수정
- module css 삭제 - vanilla-extract 변경된 코드 적용
- 테스트랑 스토리북 코드 수정
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
🚀 New features to boost your workflow:
|
export const size = styleVariants(fontSize, (value) => ({ | ||
fontSize: `${value}px`, | ||
})); |
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.
오 callback으로 값 변환이 가능하구나!
lineHeight: value, | ||
})); | ||
|
||
export const typography = recipe({ |
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.
얘는 recipe로 처리해야할 만큼 복잡한 스타일은 아니라고 생각하는데, recipe를 사용하지 않는 방향으로 작성해볼 수 있을까??
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.
알겠어!
나는 recipe 를 복잡하지않아도, 가독성이 좋아서 사용하는편인데 어떤 상황일 때 과한건지 판단하기가 어렵네..
export type LineHeight = keyof typeof lineHeightToken; | ||
|
||
export interface TypographyProps extends ComponentProps<'p'> { | ||
export interface TypographyProps extends Omit<ComponentProps<'p'>, 'color'> { | ||
as?: ElementType; |
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.
as prop은 asChild랑 동일한 역할이라서 중복 기능의 구현으로 보이는데, 추가한 이유가 있을까?
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.
as props 는 Typography 컴포넌트 자체가 특정 HTML 요소로 렌더링되도록 하고 싶을 때 사용해 h1, h2, span 등등?
asChild는 Typography 스타일을 기존의 다른 컴포넌트나 요소에 주입하거나, 불필요한 DOM 래퍼 없이 스타일을 적용하고 싶을 때 사용해.
- https://sandroroth.com/blog/react-polymorphic-components/
- https://www.radix-ui.com/primitives/docs/guides/composition
디자인 시스템 공부할 때 살펴본 문서인데, 약간의 차이때문에 구현을 통해 컴포넌트 자체의 활용도를 높일 수 있따 알고있어!
혹시 다른 의견있으면 말해줘도좋아!
'--font-size': `${fontSizeToken[size]}px`, | ||
'--font-weight': fontWeightToken[weight], | ||
'--line-height': lineHeightToken[lineHeight], | ||
color, |
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.
스타일을 직접 주입하기보다는 css variable을 활용한 방식을 그대로 유지해주는게 어떨까? @vanilla-extract/dynamic
을 활용하면 비교적 쉽게 구현할 수 있어~
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.
- 코드 리뷰 반영 - recipe 제거 - typography.tsx 파일 리팩토링
- vanilla-extract dynamic 설치 - 코드 리뷰 반영
Changes
Visuals
Checklist
Additional Discussion Points