Skip to content
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

feat: RadioGroup (RadioButton) 컴포넌트 구현 #147

Merged
merged 11 commits into from
Aug 18, 2024

Conversation

fecapark
Copy link
Collaborator

@fecapark fecapark commented Aug 11, 2024

1️⃣ 어떤 작업을 했나요? (Summary)

RadioGroup (RadioButton) 컴포넌트를 구현했습니다.

피그마상 컴포넌트 네이밍은 RadioButton이지만,
실제로 단일 RadioButton으로 사용되는 경우는 없습니다.

따라서 RadioButton들의 목록을 쉽게 관리할 수 있도록
RadioGroup 컴포넌트로 대체하여 구현하였습니다.

( 다른 컴포넌트 라이브러리들도 RadioButton 대신 RadioGroup 을 구현하는 방식으로 진행함 )

스크린샷 2024-08-12 00 34 22


기존 코드에 영향을 미치지 않는 변경사항

src/components/RadioGroup 를 추가했습니다.


기존 코드에 영향을 미치는 변경사항

시맨틱 토큰 semantic.color.lineBrandPrimary 를 추가했습니다.


버그 픽스

2️⃣ 알아두시면 좋아요!

Tabs 컴포넌트의 사용법과 동일하게, useRadioGroup 훅을 호출하여 사용하는 방식입니다.

최대한 type-safe 하도록 구현했습니다.

const RadioGroup = useRadioGroup<'한국어' | '영어' | '일본어'>();

return (
  <RadioGroup size="medium">
    <RadioGroup.Item value="한국어">한국어</RadioGroup.Item>
    <RadioGroup.Item value="영어">영어</RadioGroup.Item>
    <RadioGroup.Item value="일본어">일본어</RadioGroup.Item>
  </RadioGroup>
);

3️⃣ 체크리스트 (Checklist)

  • main 브랜치의 최신 코드를 pull 받았나요?
  • 빌드 체크 및 다른 프로젝트에서 의도한대로 작동하는지 확인

Copy link
Collaborator

@nijuy nijuy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

실제로 단일 RadioButton으로 사용되는 경우는 없습니다.
따라서 RadioButton들의 목록을 쉽게 관리할 수 있도록
RadioGroup 컴포넌트로 대체하여 구현하였습니다.

아하 group으로 구현하신 이유 이해했습니다!
고생하셨어요~~~~~🤸‍♀️✨ 당최당최

src/components/RadioGroup/RadioGroup.style.ts Show resolved Hide resolved
Comment on lines 42 to 43
button.click();
button.focus();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

키보드로 접근하면 focus는 잘 옮겨지는데 선택된 값이 변하진 않네요!
keyboard

space or enter를 누르면 선택 값도 바뀌긴 하는데, 코드에 button.click()이 있는 걸 보아 화살표 버튼만으로도 바꾸는 걸 의도하신 거 같아 코멘트 남깁니다!!

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

근데 혹시 보리 브라우저 머쓰시나영

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

저 엣지 씁니다!!

disabled={disabled}
/>

<button onKeyDown={onKeyDown}>
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

링크를 생각했을 때 현재 선택된 라디오 버튼이 아니라 첫번째 라디오 버튼에 포커스가 잡히는 점이 어색하게 느껴지는 거 같아요. button에 적절한 tabIndex를 부여하면 되긴 하는데 지금 RadioGroupItem 입장에서는 현재 선택 여부를 알 방법이 없는 거 같어서.......😶‍🌫️

저는 tab context에 currentTab을 넣어서 현재 선택된 탭을 알 수 있게 해서 tab index를 설정했었는데,
혹시 radio context에 current value가 들어가지 않은 이유가 따로 있나요??

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이유는 따로 없고 생각이 짧았네요 죄송요!!
개선해야될 점 같으니 바로 진행해볼게요!

@fecapark
Copy link
Collaborator Author

전반적으로 작성해주신 내용 종합해서,
focus 기능에 개선이 필요하다고 생각했고요.
따라서 다음과 같이 개선했어요.

2024-08-18.13.17.01-1.mov
  • tab 버튼으로 radiogroup focus는 1회만 가능하게 구현

    • 단 focus 되는 radio는 현재 active 되어있는 radio로, active된 radio가 없다면 첫번째 radio로 구현했어요.
  • 화살표 버튼으로 value 이동 가능하게 구현했어요. (value 이동시 outline 스타일 추가)

    • 이유를 모르겠는데, 화살표로 선택된 value를 이동시키면 브라우저 기본 focus-outline 스펙이 꺼져버려서 강제로 스타일에 넣어주었어요

Copy link
Collaborator

@nijuy nijuy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

고많
고생 많으셨습니다~(。・∀・)ノ゙ 라는 뜻

Comment on lines +15 to +17
size: undefined,
orientation: undefined,
currentRadioValue: undefined,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tabs 에서는 아예 여길 undefined로 넣어버려서 사용할 때 코드가 더러웠는데 ^^.............. 여기처럼 수정하겠읍니다

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

설문조사처럼 하나의 페이지 안에 n개의 radiogroup이 있어야 하는 상황을 위해
여러 개의 radioGroup을 만드는 예시도 하나 있으면 어떨까요~~!
굳이.라면 슬쩍 못본척 하고 resolve 해주세요

const LangRadioGroup = useRadioGroup<'한국어' | '영어' | '일본어'>();
const GenderRadioGroup = useRadioGroup<'남성' | '여성'>();

<LangRadioGroup size="medium">
  <LangRadioGroup.Item value="한국어">한국어</LangRadioGroup.Item>
  <LangRadioGroup.Item value="영어">영어</LangRadioGroup.Item>
  <LangRadioGroup.Item value="일본어">일본어</LangRadioGroup.Item>
</LangRadioGroup>

<GenderRadioGroup size="medium">
  <GenderRadioGroup.Item value="남성">남성</GenderRadioGroup.Item>
  <GenderRadioGroup.Item value="여성">여성</GenderRadioGroup.Item>
</GenderRadioGroup>

@fecapark fecapark merged commit 6877021 into develop Aug 18, 2024
@fecapark fecapark deleted the feat/#142-radio-button branch August 18, 2024 09:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: Radio Button 구현
2 participants