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: Switch 컴포넌트 구현 #156

Merged
merged 15 commits into from
Oct 17, 2024
Merged

feat: Switch 컴포넌트 구현 #156

merged 15 commits into from
Oct 17, 2024

Conversation

nijuy
Copy link
Collaborator

@nijuy nijuy commented Sep 20, 2024

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

image

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

  • switch 컴포넌트용 컬러 토큰을 추가했습니다

  • switch 컴포넌트를 구현했습니다

    • tab / space를 사용한 키보드 조작이 가능합니다

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

3️⃣ 추후 작업

4️⃣ 체크리스트 (Checklist)

  • main 브랜치의 최신 코드를 pull 받았나요?

@nijuy nijuy added the feat label Sep 20, 2024
@nijuy nijuy self-assigned this Sep 20, 2024
@nijuy nijuy linked an issue Sep 20, 2024 that may be closed by this pull request

export const Switch = forwardRef<HTMLDivElement, SwitchProps>(
({ isDisabled = false, isSelected = false, size, onSelectedChange, ...props }, ref) => {
const [innerSelected, setInnerSelected] = useState(isSelected);
Copy link
Contributor

Choose a reason for hiding this comment

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

이 부분 구현이 잘못됐어요.

props와 기능이 1대1 대응되는 내부 상태가 있다는 것은,
외부에서 변경된 상태가 props로 들어오더라도 이 변경점을 잘 반영해야해요.

그런데 이 코드는 해당 변경점을 잘 반영하고 있지 못한 거 같아요.

Copy link
Collaborator Author

@nijuy nijuy Sep 22, 2024

Choose a reason for hiding this comment

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

저는 isSelected의 역할이 Switch의 초기 상태(innerSelected의 초기값)라고 생각하고 만들었어요

Switch는 컴포넌트 내부 innerSelected 상태로 선택 여부를 관리하는 방식이라
외부에서 isSelected 값이 변경되어도 Switch에서는 반영하지 않는 게 맞다고 생각했스빈다

radix-ui Switch의 defaultChecked랑 같은 역할이라고 생각하시면 될 거 같아요
헷갈리지 않게 isSelected -> defaultSelected로 변경해두겠습니다!!

Copy link
Contributor

Choose a reason for hiding this comment

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

음… 납득이 안되는데요

만약 초기화버튼을 눌렀을때 switch가 false로 바뀌는 기능을 구현해야한다면

어떤 방식으로 제공할 생각이신가요?

Copy link
Collaborator Author

@nijuy nijuy Sep 29, 2024

Choose a reason for hiding this comment

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

페카 말처럼 스위치를 직접 클릭하지 않고도 상태 변경이 가능해야 하는 케이스가 있을 수 있어
isSelected 변경을 Switch에서 반영할 수 있게 코드 수정하기로 모각코때 얘기함 !!

@fecapark
Copy link
Contributor

fecapark commented Sep 21, 2024

@nijuy

++
ts-pattern 사용은 아직 안하고 있는거 같은데,
굳이 지금 추가해야할 필요 없을거 같아요.

만약 그대로 냅둬주신다면 맥락 파악을 위해서
이와 관련해서 논의한 스레드 링크가 PR 맥락으로 포함되어야할거 같아요.

@nijuy
Copy link
Collaborator Author

nijuy commented Sep 22, 2024

ts-pattern 사용은 아직 안하고 있는거 같은데,

@fecapark 제가 본문을 잘못 썼어요
처음엔 size prop별 switch 구성 요소의 크기/컬러 정할 때 ts-pattern 썼었다가
track color 결정할 때만 ts-pattern 쓰는 방식으로 수정했는데 (515b002) 착각했네요 쏘리 ! (。◠‿◠。)

- 상태 전환을 보여주는 거라 Click -> Change
- 코드 예시 추가
Copy link
Member

@seocylucky seocylucky left a comment

Choose a reason for hiding this comment

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

수고하셨습니당👍🏻👍🏻
스위치 귀엽네요
키보드로 조작할 수 있게 접근성까지 고려한 갓보리


```tsx
<span> switch label </span>
<Switch size="large">
Copy link
Member

Choose a reason for hiding this comment

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

이벤트 할당 부분 예시 코드 빼고 전부 /> 로 끝나도록 수정해야 할 것 같아요!

background-color: ${({ $isDisabled, $isSelected, theme }) =>
getTrackColor({ $isDisabled, $isSelected, theme })};
cursor: ${({ $isDisabled }) => ($isDisabled ? 'not-allowed' : 'pointer')};
`;
Copy link
Member

Choose a reason for hiding this comment

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

image

키보드로 스위치 조작 시, 위 사진처럼 파란 outline이 생기는데 focus 시, outline: none; 설정해서 없애는 거 어떨까요ㅎㅎㅎ

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

한번 적용해보니까 "현재 포커스된 요소"가 뭔지 식별이 어렵다는 문제가 있네요 🤔
대신 기본 outline이 쬐끔,,, 투박하니까 디자인에 맞게 커스터마이즈 된 outline 스타일 만들어주실 수 있는지 우미한테 물어볼게요!!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

여기 기본 outline 그대로 사용하면 된다고 답변 왔습니다!

- defaultSelected -> isSelected
- switch 컴포넌트 내부에 useUpdateEffect 추가
- 외부에서 상태 변경하는 스토리 추가
- 및 불필요한 스토리 삭제
Copy link
Member

@seocylucky seocylucky left a comment

Choose a reason for hiding this comment

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

확인했습니다~ 머지 고고링

@nijuy nijuy merged commit fae9ff6 into develop Oct 17, 2024
@nijuy nijuy deleted the feat/#152-switch branch October 17, 2024 12:09
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: Switch 컴포넌트 구현
3 participants