refactor(Tag): Tag 컴포넌트 type, icon 추가 #313
Conversation
|
|
| size?: 'sm' | 'md' | 'lg'; | ||
| shape?: 'rect' | 'pill'; | ||
| variant?: 'default' | 'primary' | 'secondary'; | ||
| type?: 'solid' | 'line'; | ||
| type?: 'solid' | 'line' | 'accent'; |
There was a problem hiding this comment.
해당 union type은 types.ts에 정의되어 있는 것 같은데 따로 사용하시지 않은 이유가 있을까요??
There was a problem hiding this comment.
사소하지만 필요하다면 args들에 간단한 description 달아주면 좋을 것 같아요
| shape = 'rect', | ||
| variant = 'default', | ||
| type = 'solid', | ||
| LeftIcon, |
There was a problem hiding this comment.
ComponentType보다는 react node를 받도록하고, 외부에서 icon 요소를 주입하도록 하면 더 좋을 것 같은데, 이와 관해 제약이 있었나요 ?
위와 같이 된다면 prop name은 leftAddon 정도로 하면 좋을 것 같아요.
|
|
||
| return ( | ||
| <div className={`${S.root} ${style} ${className}`} ref={forwardedRef} {...restProps}> | ||
| {LeftIcon ? <LeftIcon height={iconSize} width={iconSize} /> : null} |
There was a problem hiding this comment.
leftIcon 값이 없을 때 null을 리턴하는 거면 불필요한 코드를 방지하는 측면에서 &&연산자를 사용해도 좋을 것 같아보여요 !
| height: number; | ||
| } | ||
| export interface TagProps extends HTMLAttributes<HTMLDivElement> { | ||
| children?: React.ReactNode; |
There was a problem hiding this comment.
Tag 요소 특성상 children 값이 필수로 필요할 거라고 생각해요.
옵셔널로 두는 이유가 있는지 궁금합니다! 개발자의 편의를 고려한 걸까요 ??
There was a problem hiding this comment.
그냥 이전코드에서 수정되지 않은 부분인 것 같은데 children으로 사용의 확장성을 두기보다는
잘못된 사용을 방지하기 위해 text 를 interface로 받는 게 좋을 것 같다는 의견입니다!
저희 Tag는 왼쪽에 아이콘이 같이 오는 단순 읽기 전용 라벨입니다 (클릭, 포커스가 없는)
그래서 내부에서 잘못된 사용을 방지하기 위해 사용법을 단일화해야하지 않을까? 하는 의견인데 어떻게 생각하시나요?
| const iconSize = iconSizes[size]; | ||
|
|
||
| return ( | ||
| <div className={`${S.root} ${style} ${className}`} ref={forwardedRef} {...restProps}> |
There was a problem hiding this comment.
text를 받는다면 span 태그를 사용하면 좋을 것 같아요
링크
시급한 정도
🏃♂️ 보통 : 최대한 빠르게 리뷰 부탁드립니다.
기타 사항
1. type='accent' 속성 추가
accenttype 추가했습니다.accent디자인이 되어있지 않긴했는데,같은 방식으로
default-accent에 대해서도 만들어 뒀습니다. (흰배경, 검은글씨)2. LeftIcon prop 추가
3. storybook 반영
수정 내용에 맞게 Tag 스토리 파일도 반영해뒀으니, 스토리북도 한번 확인해주시면 감사하겠습니다.
추가 논의 사항
작업을 하다보니, 사진과 같이
double quote (" ")->single quote(' ')가 수정사항에 잡히는 파일들이 있었습니다.(실제 변경 사항은
gap: '4px'만 추가 됐습니다.prettier 설정 보니 single quote 가 맞는 것 같긴한데, 포맷팅 작업 한번 하면 좋을 것 같습니다