-
Notifications
You must be signed in to change notification settings - Fork 4
refactor(Tag): Tag 컴포넌트 type, icon 추가 #313
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,22 +1,40 @@ | ||
| import { forwardRef, type HTMLAttributes } from 'react'; | ||
| import * as S from './style.css'; | ||
| import createTagStyle from './utils'; | ||
| import { iconSizes } from './constants'; | ||
|
|
||
| interface IconProps { | ||
| color?: string; | ||
| width: number; | ||
| height: number; | ||
| } | ||
| export interface TagProps extends HTMLAttributes<HTMLDivElement> { | ||
| children?: React.ReactNode; | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Tag 요소 특성상 children 값이 필수로 필요할 거라고 생각해요.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 그냥 이전코드에서 수정되지 않은 부분인 것 같은데 children으로 사용의 확장성을 두기보다는 |
||
| className?: string; | ||
| size?: 'sm' | 'md' | 'lg'; | ||
| shape?: 'rect' | 'pill'; | ||
| variant?: 'default' | 'primary' | 'secondary'; | ||
| type?: 'solid' | 'line'; | ||
| type?: 'solid' | 'line' | 'accent'; | ||
|
Comment on lines
14
to
+17
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 해당 union type은 |
||
| LeftIcon?: React.ComponentType<IconProps>; | ||
| } | ||
|
|
||
| export const Tag = forwardRef<HTMLDivElement, TagProps>((props, forwardedRef) => { | ||
| const { children, className, size = 'sm', shape = 'rect', variant = 'default', type = 'solid', ...restProps } = props; | ||
| const { | ||
| children, | ||
| className, | ||
| size = 'sm', | ||
| shape = 'rect', | ||
| variant = 'default', | ||
| type = 'solid', | ||
| LeftIcon, | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ComponentType보다는 react node를 받도록하고, 외부에서 icon 요소를 주입하도록 하면 더 좋을 것 같은데, 이와 관해 제약이 있었나요 ? 위와 같이 된다면 prop name은 leftAddon 정도로 하면 좋을 것 같아요. |
||
| ...restProps | ||
| } = props; | ||
| const style = createTagStyle(type, variant, shape, size); | ||
| const iconSize = iconSizes[size]; | ||
|
|
||
| return ( | ||
| <div className={`${S.root} ${style} ${className}`} ref={forwardedRef} {...restProps}> | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. text를 받는다면 span 태그를 사용하면 좋을 것 같아요 |
||
| {LeftIcon ? <LeftIcon height={iconSize} width={iconSize} /> : null} | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. leftIcon 값이 없을 때 null을 리턴하는 거면 불필요한 코드를 방지하는 측면에서 |
||
| {children} | ||
| </div> | ||
| ); | ||
|
|
||
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.
사소하지만 필요하다면 args들에 간단한 description 달아주면 좋을 것 같아요