Refactor(Callout): 컴포넌트 문서화 및 스타일 개선 #320
Conversation
ljh0608
requested review from
constantly-dev,
imddoy,
j-nary,
rtttr1 and
wuzoo
|
|
constantly-dev
left a comment
constantly-dev
left a comment
There was a problem hiding this comment.
수고하셨습니다! 코멘트 확인해주세요 👍
There was a problem hiding this comment.
변경된 부분은 아니긴한데 아래 Icon의 visible 조건에 buttonLabel가 같이 있는데 이렇게 되면 hasIcon이 false여도 buttonLabel이 있으면 아이콘이 뜨는 case가 생길 것 같아요!
그리고 hasIcon이 optional인데 초기 값이 없어서 만약 buttonLabel을 조건에서 없애게 되면 기본 값 지정을 해주면 어떨까요?
<aside className={calloutVariant[type]}>
{buttonLabel || hasIcon ? <Icon className={iconVariant[type]} /> : null}
// ...There was a problem hiding this comment.
buttonLabel을 넣은 것은 해당 문서(이미지)를 보고 'buttonLabel이 존재하는 케이스'에서는 무조건 icon이 존재한다고 판단했기 때문이에요! 그렇게 사용되어야한다면 variant를 활용해 icon, default, buttonLabel로 나누었으면 좋았겠다는 생각이 드네요.
피그마 문서 링크도 남깁니다! 다만 Callout 디자인시스템을 플그만 사용하고 있어서 영향이 가지 않는 선에서 더 좋은 개선방안을 고민해보겠습니다!
| }, | ||
| warning: { | ||
| backgroundColor: "rgba(255, 194, 52, 0.1)", | ||
| backgroundColor: 'rgba(255, 194, 52, 0.1)', |
There was a problem hiding this comment.
해당 색상이 하드 코딩 되어있는데 저희 mds에 추가가 안되어있네요..
규홍님이 올려두신 color 관련 작업 PR에서 yellow/green Alpha 색상 추가되면 변경해주시면 좋을 것 같아요!
추가로 피그마에도 반영이 안되어있어서 yellowAlpha600으로 color 지정해달라고 디자이너 분들께 전달해주시면 감사하겠습니다
There was a problem hiding this comment.
넵 이부분 가온님께 요청드렸고 추후 머지되면 한 번 더 확인하겠습니다~
There was a problem hiding this comment.
아래 Icon에서 특별하게 스크린 리더에 담길 내용이 없다면 아이콘 2개 다 aria-hidden='true'을 달아도 좋을 것 같아요
| hasIcon: { | ||
| description: '아이콘 표시 여부를 지정합니다. buttonLabel이 있으면 이 옵션과 무관하게 아이콘이 항상 표시됩니다.', | ||
| control: 'boolean', | ||
| table: { | ||
| type: { summary: 'boolean' }, | ||
| defaultValue: { summary: 'false' }, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
story에서는 hasIcon이 default value가 false인데 아래 구현부에서는 default 값이 없어서 명시적으로 기본 값 지정하는 것은 어떨까요?
type도 마찬가지!
There was a problem hiding this comment.
hasIcon default value true로 구현부 변경하고 해당 스토리의 defaultValue도 변경하였습니다~
type은 이 컴포넌트를 사용하는 유저가 명확히 타입을 지정해주었으면 하는 의도로 디폴트 값을 넣지 않았는데 type 스토리에 잘못 기입된 것 같아서 이부분도 반영했습니다 감사합니다 😃
08f4619
There was a problem hiding this comment.
아하! 그런 의도로 그대로 작성하신거였군요! 👍
wuzoo
left a comment
wuzoo
left a comment
There was a problem hiding this comment.
진혁님이 말씀주신 것들만 해결되면 머지해도 좋을 것 같네용. 고생하셨습니다 !
변경사항
링크
Callout Figma Link
https://www.figma.com/design/ZuAGH1zZDxduHsjCZm8kfH/%F0%9F%9A%80-makers-design-system?node-id=9195-10174&m=dev
시급한 정도
🐢 천천히 : 급하지 않습니다.
기타 사항
피그마상 width: 358의 fixed 디자인이지만 현재 플그에서 사용하고 있는 UI에 영향이 가기 때문에 짧은 텍스트에 대한 대응을 위해 minWidth을 적용하였습니다.
AS-IS
TO-BE