Conversation
| import * as styles from "./badge.css"; | ||
|
|
||
| interface BadgeProps { | ||
| status: "forSale" | "soldOut"; |
There was a problem hiding this comment.
status를 이렇게 고정해버리면 "판매중 여부"라는 도메인에 강결합되어버려서 재사용성이 떨어져요. 나중에 동일 Badge가 판매랑 상관없는 다른 도메인에서 사용되면 forSale/soldOut이라는 status를 가지는 게 어색해집니다. 뱃지의 UI를 기준으로 보았을 때는 활성화/비활성화 두 가지 상태를 가지므로 active: boolean의 인터페이스를 가지는 것이 더 자유롭고 자연스러워요.
만약 "판매중 여부"에만 사용되는게 확실하고, 확장성 없이 이 도메인에 의도적으로 종속되게끔 하고 싶다면 지금처럼 Badge라는 네이밍보다는 SalesBadge처럼 컴포넌트명에 특정 도메인에서만 사용된다는 의미가 들어가는 것이 좋아보입니다!!
There was a problem hiding this comment.
말씀해주신 부분 공감합니다!
현재 디자인 기준으로는 판매 상태에만 사용될 예정이어서 SalesBadge로 네이밍을 변경하겠습니다!!
|
|
||
| interface BadgeProps { | ||
| status: "forSale" | "soldOut"; | ||
| children: React.ReactNode; |
There was a problem hiding this comment.
뱃지 구조상 내부에 text밖에 못 들어올 것 같은데 그럼 요로케 하는 건 어떨까요?!
| children: React.ReactNode; | |
| text: string |
| interface ProductInfoProps { | ||
| status?: "forSale" | "soldOut"; | ||
| } |
There was a problem hiding this comment.
요거 status가 다양하게 들어올 수 있거나 추후 확장 가능 여부가 있으면 지금처럼 string 리터럴 유니온으로 하는 게 좋은데, 지금은 사실 판매중/판매완료 둘 중 하나이기 때문에 isSoldout: boolean 같이 boolean으로 제어해도 괜찮겠네요!!
++ 사실 이건 서버에서 어떻게 내려오냐에 따라 달라질 것 같은데, 서버에서는 isSell: boolean으로 내려주고 있어요. 그러면 저희도 이에 맞게 boolean으로 관리하는 것이 훨씬 편합니다!
There was a problem hiding this comment.
헉 미처 확인하지 못했네요 좋습니다 감사합니다👌🏻🙌🏻
| right={<MoreButton />} | ||
| /> | ||
| <ProductInfo /> | ||
| <ProductInfo status="soldOut" /> |
There was a problem hiding this comment.
판매중/판매완료 상태를 optional prop으로 설계하여 Badge 노출 여부를 부모에서 제어하도록 개선했습니다.
=> 판매중/판매완료 상태를 부모에서 제어해야 하는 이유가 궁금해요!
There was a problem hiding this comment.
ProductInfo는 채팅 페이지와 평가 페이지에서 공통으로 사용되고 있으며,
평가 페이지에서는 SalesBadge를 노출하지 않습니다.
따라서 화면에 따라 Badge 노출 여부를 제어할 수 있도록 optional prop으로 설계했습니다~!
jungwoo3490
left a comment
jungwoo3490
left a comment
There was a problem hiding this comment.
구두로 설명드린 내용 정리드립니다!!
SalesBadge라고 "판매"라는 도메인에 종속시킨 이상, 뱃지 텍스트는 isSoldout prop에 의해 결정돼요.
isSoldout: true - 판매완료
isSoldout: false - 판매중
라는 것을 이미 저희는 알고 있죠.
이런 상황에서는 isSoldout과 text prop을 두 개 두는 것이 개발자 입장에서 불편하기도 하고, 실수로 매핑을 잘못해서 휴먼 에러가 발생할 가능성도 있어요. 이런 경우는 isSoldout 이라는 단일 인터페이스만 뚫고, 텍스트는 isSoldout에 따라 내부에서 계산되도록 하는 것이 편리하고 실수 가능성도 적어집니다!
하지만 도메인에 종속시키지 않고 Badge라고 한다면, Badge 입장에서는 어떤 텍스트가 활성화에 매핑되고, 어떤 텍스트가 비활성화에 매핑되는지 알지 못해요. 그러므로 텍스트로 뭐가 들어올지 모르는 이런 경우에는 isActive와 text를 함께 뚫어주는 게 맞습니닷!
잘 반영해주셨네요~!
| <div className={styles.ProductTitleAndStatus}> | ||
| <p className={styles.ProductTitle}>라탄 자전거 바구니</p> | ||
| <div className={styles.ProductStatus}>거래중</div> | ||
| {isSoldOut != null && <SalesBadge isSoldOut={isSoldOut} />} |
There was a problem hiding this comment.
왜
{isSoldOut && <SalesBadge isSoldOut={isSoldOut} />}이 친구는 안되고 얕은 비교(!=)를 사용해야 하는지 공부해보면 얕은 비교 원리와 단락평가에 대해 잘 이해하게 될거예요!!
There was a problem hiding this comment.
++ 더 간단하게 널병합 연산자를 사용해도 좋아보입니닷~!
| {isSoldOut != null && <SalesBadge isSoldOut={isSoldOut} />} | |
| {isSoldOut ?? <SalesBadge isSoldOut={isSoldOut} />} |
There was a problem hiding this comment.
[공부 내용 정리]
&& 단락 평가를 사용할 경우 isSoldOut이 false일 때도 렌더링이 되지 않는다.
그러나 isSoldOut === false는 판매중을 의미하는 유효한 상태값이므로 렌더링되어야 한다.
따라서 null과 undefined인 경우만 제외하기 위해 isSoldOut != null 조건으로 수정했다.
++ != null은 JS의 느슨한 비교 특성상 null과 undefined를 동일하게 판단하기 때문에, 두 값을 한 번에 제외할 수 있다.
2 participants
🔥 Related Issues
✅ 작업 내용
뱃지 컴포넌트를 구현했어요!
판매중/판매완료 상태를 optional prop으로 설계하여 Badge 노출 여부를 부모에서 제어하도록 개선했습니다.
📸 스크린샷 / GIF / Link