-
Notifications
You must be signed in to change notification settings - Fork 2
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
[FE] feat : 받은, 작성한 리뷰 없을 때 보여주는 돋보기 컴포넌트 생성 #1024
base: develop
Are you sure you want to change the base?
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.
돋보기 배율을 고려한 작업이 인상적이네요!!
궁금한 점들 코멘트 남겼어요~~
@@ -0,0 +1,32 @@ | |||
import styled from '@emotion/styled'; | |||
|
|||
export const EmptyContent = styled.div<{ $iconMessageGap?: string }>` |
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.
emotino에서 js 값을 가져올 때 Pick<EmptyContentProps, '$iconMessageGap'>
처럼 명시적으로 가져올 수 있을 것 같은데, 이렇게 리터럴을 사용한 이유가 있나요?
그리고 문서에 나와있는지는 모르겠지만 😅 styled에 값을 전달할 때 리터럴을 쓰지 않기로 했었던 것 같습니다~!
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.
제 기억으로는 모든 스타일 컴포넌트 props에 대해 타입 선언을 하면 코드가 불필요하게 길어져서 dx상 불편할 수 있어서, props가 두 개 이상일 경우 가독성 측면에서 타입으로 따로 분리하자고 했었어요. 이 부분은 정확하게 한번 정리헤야겠네요.
현재 EmptyContentProps에서는 iconMessageGap이라는 키를 사용 중이에요.
키가 $iconMessageGap 이라고 사용할 때는 가정하는 질문이라면 코드 가독성 측면에서 스타일 컴포넌트에서는 Pick을 잘 사용하지 않아요. (해당 타입이 복잡하다면 고려해 볼 수 있겠지만). 필요한 props, 해당 타입을 직관적으로 알 수 있죠. 또한 Pick 사용 시 계산 비용이 있어서 스타일 컴포넌트에서는 되도록 필요한 props 만 전달하는게 계산측면에서도 재사용 측면에서 (외부에 독립적일 수 있음) 좋다고 알고 있어요.
올리가 의미하는 명시적이라는 것은 "해당 스타일 컴포넌트가 사용하는 컴포넌트의 props 중 이거를 받는다" 라는 의미인가요?
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.
맞습니다! 하나의 값만 사용하는데 인터페이스를 선언하면 가독성이 떨어져서 리터럴을 사용한 걸까 ? < 라는 생각이 들어서 명시적이라는 말을 붙였어요.
컨벤션 문서화의 중요성을 새삼 깨닫네요 🙃🙃 자꾸 메모리 이슈가...
aspect-ratio: 39/25; | ||
width: ${(props) => { | ||
if (props.$width && props.$height) return props.$width; | ||
if (props.$width) return props.$width; | ||
return 'auto'; |
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.
본문의 의도를 이렇게 이해했는데 맞을까요?
- width, height 모두 존재 -> 무조건 그 값에 따름
- 하나만 존재 -> 받은 값을 그대로 사용, 나머지 하나의 값을 이 값을 기반으로 설정함
- 둘 다 없음 -> aspect-ratio 동작 x
이게 맞다면 기존 aspect-ratio의 동작 원리와 같아서 if문을 거의 사용하지 않아도 될 것 같은데 명시적으로 사용한 이유가 궁금해요~
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.
둘 다 없으면 height의 기본값이 적용되고, hight의 기본값에 비율이 적용된 width가 자동으로 적용돼요.
aspect-ratio는 with,height 둘 다 값이 있을 경우에만 작동하지 않아요.
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.
앗 이제 이해했어요!! early return이라 이렇게 하지 않으면 누락되는 케이스가 생기겠네요
🚀 어떤 기능을 구현했나요 ?
🔥 어떻게 해결했나요 ?
돋보기 아이콘 사이즈
돋보기 아이콘의 경우, 피그마의 사진 비율인
39/25
을 최대한 사용하도록 했습니다.height,width를 props로 받을 수 있지만 둘 중 하나만 있는 경우 props로 내려준 값을 기준으로 39/25 비율로 나머지 값도 자동으로 맞추어지도록 설계했습니다.
만약 height.width 둘 다 있는 경우는 둘 다 적용되며, 둘 다 없는 경우는 기본값이 적용됩니다. (수치 기본값은 아래를 참고해주세요)
수치 기본값
다양한 사이즈, 메세지의 컴포넌트 사용 예시
사진
코드
📝 어떤 부분에 집중해서 리뷰해야 할까요?
📚 참고 자료, 할 말