[Refactor/#362] 메인 및 예매 조회 UI 변경사항 반영 #371
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
xsmall 버튼만 유동적으로 사용하는 것으로 알고 있어 사이즈 커스텀 시 폰트는 xsmall과 동일하게 지정해두었습니다
3 tasks
|
PR 작성하느라 고생 많았어요!! 라벨 잘 지정되었는지 확인 한 번 해 주기 🫶 |
imddoy
approved these changes
Aug 29, 2024
Comment on lines
62
to
68
| } | ||
| } else if (typeof $size === "object") { | ||
| return css` | ||
| width: ${$size.width}; | ||
| height: ${$size.height}; | ||
| ${({ theme }) => theme.fonts["caption1-semi"]}; | ||
| `; |
There was a problem hiding this comment.
p5) 뜨거운 감자였는데 좋은 선택인것 같습니다!
Comment on lines
34
to
35
| width: 32.7rem; | ||
| height: 5.6rem; |
There was a problem hiding this comment.
p3) 위에서 width랑 height를 따로 정의해뒀는데 이렇게 사용하는 것은 별로 좋은 선택은 아닌 것 같습니다..!
아래에 코드 작성해둘테니 변경하는 것도 좋을 것 같습니다.
Comment on lines
41
to
43
| height: 4.6rem; | ||
| ${({ theme }) => theme.fonts["body2-normal-semi"]}; | ||
| `; |
There was a problem hiding this comment.
p3) 보니까 fonts가 중복되는 case들이 많고, theme을 계속 가져와서 사용하는 것이 중복되는 것 같습니다.
얘도 마찬가지로 아래에 코드 작성할테니 맘에 드시면 변경해주세요!
Comment on lines
29
to
62
| ${({ $size }) => { | ||
| switch ($size) { | ||
| case "xlarge": | ||
| return css` | ||
| ${({ theme }) => theme.fonts["body1-normal-semi"]}; | ||
| `; | ||
| case "large": | ||
| return css` | ||
| ${({ theme }) => theme.fonts["body2-normal-semi"]}; | ||
| `; | ||
| case "medium": | ||
| return css` | ||
| ${({ theme }) => theme.fonts["body2-normal-semi"]}; | ||
| `; | ||
| case "small": | ||
| return css` | ||
| ${({ theme }) => theme.fonts["body2-normal-semi"]}; | ||
| `; | ||
| case "xsmall": | ||
| return css` | ||
| ${({ theme }) => theme.fonts["caption1-semi"]}; | ||
| `; | ||
| if (typeof $size === "string") { | ||
| switch ($size) { | ||
| case "xlarge": | ||
| return css` | ||
| width: 32.7rem; | ||
| height: 5.6rem; | ||
| ${({ theme }) => theme.fonts["body1-normal-semi"]}; | ||
| `; | ||
| case "large": | ||
| return css` | ||
| width: 27.9rem; | ||
| height: 4.6rem; | ||
| ${({ theme }) => theme.fonts["body2-normal-semi"]}; | ||
| `; | ||
| case "medium": | ||
| return css` | ||
| width: 15.8rem; | ||
| height: 4.6rem; | ||
| ${({ theme }) => theme.fonts["body2-normal-semi"]}; | ||
| `; | ||
| case "small": | ||
| return css` | ||
| width: 13.6rem; | ||
| height: 4.6rem; | ||
| ${({ theme }) => theme.fonts["body2-normal-semi"]}; | ||
| `; | ||
| case "xsmall": | ||
| return css` | ||
| width: 10.3rem; | ||
| height: 3.6rem; | ||
| ${({ theme }) => theme.fonts["caption1-semi"]}; | ||
| `; | ||
| } |
There was a problem hiding this comment.
p3) 여기까지 작성된 if문을 아래 코드로 변경하는 것은 어떤가용??
계속 중복되는 폰트들도 많고, theme도 반복해서 가져오고, width를 직접 하나하나 작성하는 것이 비효율적인 것 같아서 어차피 js 문법을 사용할거면 font를 따로 변수로 만들어서 정의하고 한번에 return하는 것이 깔끔할 것 같습니다!!
${({ $size, theme }) => {
if (typeof $size === "string") {
let font;
switch ($size) {
case "xlarge":
font = theme.fonts["body1-normal-semi"];
break;
case "xsmall":
font = theme.fonts["caption1-semi"];
break;
default:
font = theme.fonts["body2-normal-semi"];
}
return css`
${font};
width: ${width[$size]};
height: ${height[$size]};
`;
} 
There was a problem hiding this comment.
헉 그렇네요........!! 반영하겠습니다 레전드띠예
pepperdad
approved these changes
Aug 29, 2024
Comment on lines
29
to
62
| ${({ $size }) => { | ||
| switch ($size) { | ||
| case "xlarge": | ||
| return css` | ||
| ${({ theme }) => theme.fonts["body1-normal-semi"]}; | ||
| `; | ||
| case "large": | ||
| return css` | ||
| ${({ theme }) => theme.fonts["body2-normal-semi"]}; | ||
| `; | ||
| case "medium": | ||
| return css` | ||
| ${({ theme }) => theme.fonts["body2-normal-semi"]}; | ||
| `; | ||
| case "small": | ||
| return css` | ||
| ${({ theme }) => theme.fonts["body2-normal-semi"]}; | ||
| `; | ||
| case "xsmall": | ||
| return css` | ||
| ${({ theme }) => theme.fonts["caption1-semi"]}; | ||
| `; | ||
| if (typeof $size === "string") { | ||
| switch ($size) { | ||
| case "xlarge": | ||
| return css` | ||
| width: 32.7rem; | ||
| height: 5.6rem; | ||
| ${({ theme }) => theme.fonts["body1-normal-semi"]}; | ||
| `; | ||
| case "large": | ||
| return css` | ||
| width: 27.9rem; | ||
| height: 4.6rem; | ||
| ${({ theme }) => theme.fonts["body2-normal-semi"]}; | ||
| `; | ||
| case "medium": | ||
| return css` | ||
| width: 15.8rem; | ||
| height: 4.6rem; | ||
| ${({ theme }) => theme.fonts["body2-normal-semi"]}; | ||
| `; | ||
| case "small": | ||
| return css` | ||
| width: 13.6rem; | ||
| height: 4.6rem; | ||
| ${({ theme }) => theme.fonts["body2-normal-semi"]}; | ||
| `; | ||
| case "xsmall": | ||
| return css` | ||
| width: 10.3rem; | ||
| height: 3.6rem; | ||
| ${({ theme }) => theme.fonts["caption1-semi"]}; | ||
| `; | ||
| } |
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
📌 관련 이슈번호
🎟️ PR 유형
어떤 변경 사항이 있나요?
✅ Key Changes
📢 To Reviewers
<Button variant="line" size={{ width: "10.8rem", height: "3.6rem" }} >처럼 size에 width와 height를 주면 xsmall 폰트에 맞춰 사이즈 변경할 수 있도록 수정하였습니다!📸 스크린샷