-
Notifications
You must be signed in to change notification settings - Fork 6
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
시대생 프론트엔드 과제 1 PR 이소은 #5
base: master
Are you sure you want to change the base?
Conversation
Features
Tests
Commit-Lint commandsYou can trigger Commit-Lint actions by commenting on this PR:
|
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.
LGTM~
<S.cafeteriaWrapper> | ||
<S.locationWrapper> | ||
<Text | ||
text={cafeteria.location} |
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.
태그 사이에 들어갈 요소가 없다면 '< Text />' 로 간결하게 태그를 표현하는 방법도 있습니당
text={cafeteria.openingHours} | ||
fontSize={15} | ||
fontWeight={500} | ||
color="#808A98" |
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.
반복적인 color 값이 들어간다면 해당 값을 상수로 정하시거나, config 폴더를 따로 제작하여 값을 관리하는 방법도 좋아 보여요
return ( | ||
<S.textWrapper width={width} lineHeight={lineHeight}> | ||
<S.text | ||
text={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.
styled-component로 제작된 S.text 컴포넌트에 text 속성이 사용되어지지 않기에, 필요하지 않는 props라고 생각됩니다.
Features
Test