-
Notifications
You must be signed in to change notification settings - Fork 4
refactor(Tab): Tab 컴포넌트 storybook 및 접근성 개선 #286
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?
Conversation
|
|
ljh0608
left a comment
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.
PR이 상세해서 많이 배우기도 했고 이해하기 좋았어요 감사합니다~
| {translator ? translator[item] : item} | ||
| </button> | ||
| <div className={`${S.tabItemUnderline} ${isSelected}`} /> | ||
| <div className={`${S.tabItemUnderline} ${isSelected ? 'selected' : ''}`} /> |
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.
selected className이 어떤 용도로 활용되는건가요??
이부분도 중보되고 있어서 상단에 변수로 선언해두면 좋을 것 같아요
const selectedClassName= isSelected ? 'selected' : ''
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.
수정하신 isSelected 문제 없어보이네요.
그리고 클래스네임은 빈 문자열보다는 null (없음)은 어떠신지
위 재훈님이 말씀하신 것도 동의합니다
| selectedInitial: { control: 'select', options: ['Tab1', 'Tab2', 'Tab3'] }, | ||
| tabItems: { | ||
| description: '여러 Tab 항목을 전달합니다.', | ||
| table: { type: { summary: 'T[]' } }, |
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.
story는 구체적인 타입이나 예시를 넣는것이 문서를 읽는데 더 좋은 방향인 것 같아요 제네릭 타입보단 string[] 을 사용해보는건 어떤가요?
| table: { type: { summary: 'T (optional)' } }, | ||
| }, | ||
| translator: { | ||
| description: 'Tab 항목의 텍스트를 원하는 컴포넌트로 변경합니다.', |
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.
Tab 항목의 텍스트를 ReactNode로 매핑합니다. (아이콘, 강조 텍스트 등 가능)
와 같이 ReactNode 사용을 명시해줘도 좋을 것 같아요!
| size: { | ||
| control: 'inline-radio', | ||
| options: ['sm', 'md', 'lg'], | ||
| description: 'Tab의 크기를 설정합니다.', | ||
| table: { type: { summary: 'sm | md | lg' } }, | ||
| }, |
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.
해당 문서에 defaultValue가 빠진 것 같아요! 이부분 추가해주세요!
wuzoo
left a comment
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.
고생하셨습니다 ~!
| {translator ? translator[item] : item} | ||
| </button> | ||
| <div className={`${S.tabItemUnderline} ${isSelected}`} /> | ||
| <div className={`${S.tabItemUnderline} ${isSelected ? 'selected' : ''}`} /> |
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.
수정하신 isSelected 문제 없어보이네요.
그리고 클래스네임은 빈 문자열보다는 null (없음)은 어떠신지
위 재훈님이 말씀하신 것도 동의합니다
변경사항
링크
시급한 정도
🏃♂️ 보통 : 최대한 빠르게 리뷰 부탁드립니다.
기타 사항
1️⃣ 내부 isSelected 일부 변경
기존 코드를 살펴보니
isSelected가 아래와 같이 설정된 것을 볼 수 있었습니다.물론 반복되는 코드를 줄이기 위해 사용한 것은 좋지만,
is-prefix의 의미가 대부분boolean값을 가진다는 점과aria-selected에서 선택 여부에 해당하는boolean값을 필요로 한다는 것을 근거로 삼아 코드를 아래와 같이 수정하였습니다.Tab의 interface를 바꾸지 않아 크게 문제가 될 것이 없다고 생각하지만, 이 부분에 대해서 잘못된 방향이 있다면 코멘트 부탁드립니다!
2️⃣ tabIndex 사용 접근성 개선
모든 탭이 tab key로 이동되어도 되지만, 접근성을 고려할 때 활성화 되는 탭을 이동시켜 주는 것이 좋다고 판단했습니다.
또한 material design 코드에도
tabIndex가 tab bar에 적용된 것을 근거로 사용하는 것이 접근성에 더 좋다고 판단하여 추가하였습니다.material design - tab bar 링크
2025-09-30.7.02.11.mov
3️⃣ storybook show code 컴포넌트 이름 이슈
storybook에서 show code를 누르면 컴포넌트명이 이상하게 뜨는 이슈가 있었습니다. 이 부분은 구현 컴포넌트의 이름을 제대로 인식하지 못하는 이유라고 판단해서 처음에는 import/export문을 수정해보기도 했는데 결국 해결한 방법은 아래와 같습니다.
Tab 컴포넌트 파일 최하단에
displayName을 지정해줘서 해결했습니다. 이 부분에 대해서 storybook github issue등에서 찾아보니 뭔가 해결됐다고 코멘트를 남겼지만, 제대로 해결이 안된 사람들이 많은 것 같았습니다. 그래서 issue안에서 제일 많이 해결하는 방법 중 하나가 displayName 지정이라고 판단해서 추가했는데, 더 좋은 의견 있으시다면 알려주시면 감사하겠습니다.storybookjs/storybook#20920
storybookjs/storybook#18972
Note
Tab 컴포넌트 스토리북에서
WithComponent에 대한 상황은 지금 기준으로 어드민에서만 사용하는 것 같습니다.이 부분이 아직 mds 피그마에는 반영이 안된 것 같아서 PD분들께 말씀드려 이번주 내로 회의 후 작업 진행하겠다는 답변을 받았습니다.
이후 추가로 mds에 제가 작업할 부분이 있다면 답변 받는대로 작업하겠습니다!