[Feat] BDYFE-183 Chip Fill 타입 추가 구현 - #180
Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
winchoose
left a comment
There was a problem hiding this comment.
고생하셨습니다!! 필수적인건 아니지만 코멘트 하나만 확인해주세요!
There was a problem hiding this comment.
ChipButton에서 disabled와 active가 동시에 들어올 수 있는데 이 경우 시각적으로는 disabled 스타일이 우선 적용되지만 aria-pressed는 여전히 active 값을 따릅니다. 따라서 비활성 상태에서도 선택된 상태로 인식될 수 있을 것 같아요
There was a problem hiding this comment.
Figma의 변경된 Chip 스펙을 확인하다가 타이포그래피 부분은 디자인 측에 한 번 확인이 필요해 보여요!
현재 스펙상 Line/Medium은 Default와 Pressed 모두 body-m-15를 사용하고 있는데, 새로 추가된 Fill/Medium은 Default에서 body-r-14, Pressed에서 body-sb-14를 사용하고 있어요. 같은 Medium 크기이지만 타입과 상태에 따라 글자 크기와 굵기가 달라지는 구조로 되어 있어요.
기존에는 버튼 사이즈에 따라 타이포그래피가 동일했기 때문에 아래처럼 정의해두고 사용할 수 있었지만, 만약 이번에 변경된 스펙처럼 같은 사이즈라 하더라도 type에 따라 타이포그래피가 달라진다면 코드를 수정할 필요가 있을 것 같아요.
const chipSizeStyles = {
sm: 'px-3 text-body-r-14',
md: 'px-4 text-body-m-15',
};현재 구현은 size="md"일 때 공통으로 body-m-15를 적용하고 Fill은 활성 색상만 변경하고 있어서, 지금 Figma 스펙과는 일치하지 않아요.
일단 무조건적으로 수정하기 전에 Fill에서만 타이포그래피를 다르게 사용하려는 의도가 맞는지 디자인 측에 먼저 확인해 보면 좋을 것 같아요. 의도된 스펙이라면 Fill의 Default/Pressed 스타일을 별도로 분리해서 적용하고, 아니라면 Figma 스펙을 정리한 뒤 그 기준에 맞추면 될 것 같습니다!!
| chipSizeStyles[size], | ||
| active && chipActive, | ||
| active && chipTypeStyles[variant], | ||
| disabled && chipDisabled, |
There was a problem hiding this comment.
여기서 disabled를 비활성 스타일을 적용하는 데만 사용하고, 실제 <button>에는 전달하지 않고 있는 것 같아요.
<button disabled={disabled}>가 되도록 native 속성도 함께 전달해 주면 좋을 것 같습니다 ~~
🔗 Jira 이슈키
📌 Summary
Chip 컴포넌트에 active 스타일을 선택할 수 있는
fillvariant를 추가했습니다.📚 Tasks
line,fillvariant 추가Chip과ChipButton에variantprop 추가🔍 Describe
variant='fill'과active를 함께 전달하면 스타일이 적용됩니다.기존 사용처에 영향을 주지 않도록 기본값은 기존 활성화 스타일인
line으로 유지했습니다.🖼️ Screenshot
👀 To Reviewer