-
Notifications
You must be signed in to change notification settings - Fork 4
refactor(flex): apply vanilla-extract #163
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
- CSS 클래스로 변환 - 스토리북에 grow, shrink, basis 추가 - 일부 동적요소 대응하도록 수정
|
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 (
|
- build 버그 수정
Codecov ReportAttention: Patch coverage is
🚀 New features to boost your workflow:
|
packages/flex/src/Flex.tsx
Outdated
align && styles.align[align], | ||
justify && styles.justify[justify], | ||
wrap && styles.wrap[wrap], | ||
inline && styles.display['inline-flex'], |
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.
styles.base에 display:flex가 default 값이고, inline flag가 세워지면 inline-flex로 오버라이딩이 되는 방식인 것 같은데, 그 의도를 파악하기 어려웠달까요.
inline ? styles.display['inline-flex'] : styles.display['flex']
or
inline ? styles.display['inline-flex'] : styles.base
이런식으로 하는건 어떤가효...? styles.base가 아직 display: flex만 잡고있기에 가능하긴 합니다만.
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.
흠.. 그럴 수 있겠네
그럼 base 를 조건부 거는게 아니라 inline 조건부 걸어서 CSS 오버라이드에 의존하지 않도록 해야겠다!
Co-authored-by: Evan <[email protected]>
- 명시적 조건부 렌더링 추가
packages/flex/src/Flex.tsx
Outdated
direction?: 'row' | 'column' | 'row-reverse' | 'column-reverse'; | ||
align?: 'flex-start' | 'flex-end' | 'center' | 'stretch' | 'baseline' | 'normal'; | ||
justify?: 'flex-start' | 'flex-end' | 'center' | 'space-between' | 'space-around' | 'space-evenly' | 'normal'; | ||
wrap?: 'nowrap' | 'wrap' | 'wrap-reverse'; |
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.
Flex.css.ts에서 정의된 variant를 여기서 다시 한번 하드코딩으로 집어 넣게되면, 나중에 변경되었을때 유지보수가 힘들 것 같은데, Flex variant를 같이 공유해서 타입을 잡을 수는 없을까요 ?
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.
오케!
상수 파일에서 통합관리 하도록 분리했어! 땡큐
- 코드 리뷰 반영 - constants 로 상수값 관리 - lint 버그 수정
Changes
Visuals
Checklist
Additional Discussion Points