-
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] 수강신청 페이지 & 내 과제 페이지 마크업 #24
Conversation
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.
엄청 빠르시네용 🥰 마이너 리뷰 한번 확인해주시면 감사하겠스빈당
@eugene028
각 페이즈의 Table 마다 내부 컴포넌트는 거의 다른데 요런 컨텐츠 영역은 대부분 유사한 거 같아 Table.Content 컴포넌트 만들어두었습니다 (text,subText,textRight)
|
빠른 작업 감사합니다!! |
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.
수고하셨습니다...!
<Box | ||
text={ | ||
<> |
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.
p4;
Box 컴포넌트가 props를 받을 때 children 방식이 아니라 text로 받고 있어서 어색한 거 같다는 생각이 들긴 하네요...
그래서 불필요한 wrapper 태그 (Fragment)를 추가해줘야 하는 거 같습니다...
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.
음음 Box 에 들어가는 내용들이 점점 많아지면서 text
보다 children 으로 받는 게 더 맞는 거 같은.. 생각이 드네요. 요것도 컴파운드 패턴으로 수정해주면 코드의 가독성이 개션될 것 같아요
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.
일단 머지하고 추후 태스크로 개선해보면 좋을 거 같긴 합니다!
Box 컴포넌트 개발해주신 유진님은 어떻게 생각하시나요?
@eugene028
@@ -3,7 +3,7 @@ | |||
"private": true, | |||
"scripts": { | |||
"build": "turbo build", | |||
"codegen:build": "pnpm ui:codegen && pnpm admin:codegen && pnpm client:codegen && pnpm styled-system:format", | |||
"codegen:build": "pnpm ui:codegen && pnpm ui:cssgen && pnpm admin:codegen && pnpm client:codegen && pnpm styled-system:format", |
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.
ui 가 변경됨에 따라 cssgen 도 계속 해주어야 돼서 우선 codegen:build 스크립트에 추가해두었어용,,
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.
수고하셨습니다!
일단 머지하시죠~
🎉 변경 사항
🚩 관련 이슈
🙏 여기는 꼭 봐주세요!
(wow-icon 에 아이콘 추가가가 필요해서 남은 마크업 작업과 같이 다른 PR 에서 진행해볼게요)-> 추가완료+) 만들다보니 Space 컴포넌트도 필요할 것 같아서 추가로 만들어볼게용