[FE] Checkbox component 변경 #960
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,28 +1,56 @@ | ||
| /** @jsxImportSource @emotion/react */ | ||
| import {forwardRef, useState} from 'react'; | ||
|
|
||
| import {useTheme} from '@components/Design/theme/HDesignProvider'; | ||
|
|
||
| import {IconCheck} from '../Icons/Icons/IconCheck'; | ||
|
|
||
| import {boxStyle, checkboxStyle, invisibleInputStyle} from './Checkbox.style'; | ||
| import {CheckboxProps} from './Checkbox.type'; | ||
|
|
||
| const Checkbox = forwardRef<HTMLInputElement, CheckboxProps>( | ||
| ({right, checked: controlledChecked, onChange, defaultChecked = false, ...props}, ref) => { | ||
| const {theme} = useTheme(); | ||
| const [internalChecked, setInternalChecked] = useState(defaultChecked); | ||
|
|
||
| const isControlled = controlledChecked !== undefined; | ||
|
There was a problem hiding this comment. 비제어 컴포넌트를 고려하신 이유가 궁금해요! 제가 생각하는 비제어 컴포넌트의 장점은 state의 prop drilling 없이 form submit 시 name으로 값을 가져올 수 있다는 것이에요. 혹시 이점을 고려해서 만든 것인지 아니면 토다리가 생각하는 다른 장점이 있어 비제어 컴포넌트 방식을 고려했는지 궁금합니다~ There was a problem hiding this comment. 제어 컴포넌트와 비제어 컴포넌트 각각의 장점이 있겠지만, 대부분 상황에서 제어 컴포넌트를 사용할 것 같아요. 그렇지만, 비제어 컴포넌트의 가능성을 아에 닫아두는것 보단, 일단 열어두고 사용할 수 있다면 사용할 수 있는게 맞다고 판단했습니다 There was a problem hiding this comment. 추후에 비제어로 접근할 수도 있으니 열어둔다는 의미였군요. Checkbox는 공통 컴포넌트이니깐 사용성을 열어둔다.. 좋은 것 같아요! |
||
| const checked = isControlled ? controlledChecked : internalChecked; | ||
|
|
||
| const handleChange = (e: React.ChangeEvent<HTMLInputElement>) => { | ||
| if (!isControlled) { | ||
| setInternalChecked(e.target.checked); | ||
| } | ||
|
Comment on lines
+20
to
+23
There was a problem hiding this comment. 나중을 위해 비제어까지 열어서 구현하신게 정말..👍 |
||
| onChange?.(e); | ||
| }; | ||
|
|
||
| const handleKeyDown = (e: React.KeyboardEvent) => { | ||
|
|
||
| if (e.key === ' ' || e.key === 'Enter') { | ||
| e.preventDefault(); | ||
| const input = e.currentTarget.querySelector('input'); | ||
| if (input) { | ||
| input.click(); | ||
|
Comment on lines
+29
to
+32
There was a problem hiding this comment. 오 내부의 input을 찾아서 클릭 좋네요 |
||
| } | ||
| } | ||
| }; | ||
|
|
||
| return ( | ||
| <label css={checkboxStyle} role="checkbox" aria-checked={checked}> | ||
|
There was a problem hiding this comment. 접근성까지 야무지게 챙겨가는 토다리 좋습니다~ |
||
| <div css={boxStyle({theme, checked})} aria-hidden="true" tabIndex={0} onKeyDown={handleKeyDown}> | ||
| {checked && <IconCheck size={20} color="onPrimary" />} | ||
| <input | ||
| ref={ref} | ||
| type="checkbox" | ||
| checked={checked} | ||
| onChange={handleChange} | ||
| css={invisibleInputStyle} | ||
| aria-label="체크박스" | ||
|
There was a problem hiding this comment. 어떤 체크박스인지 aria-label에 넣어주면 더 좋을 것 같아요! 예를 들어, [x] 소하 There was a problem hiding this comment. 스토리북에서 확인했을 때는 '소하 체크박스' 이런식으로 잘 읽히네요! 대신에 '체크박스 소하 체크박스' 이런식으로 두번 읽히네욤! There was a problem hiding this comment. 그러면, aria-label이 있으면, 그대로 내보내고 없다면 체크박스 가 되는 식으로 하는게 나을 것 같네요! |
||
| {...props} | ||
| /> | ||
| </div> | ||
| {right ?? null} | ||
|
There was a problem hiding this comment. right가 없이 undefined 상태에서도 그대로 없는 상태로 렌더 됩니다 There was a problem hiding this comment. 4d2f7e6 |
||
| </label> | ||
| ); | ||
| }, | ||
| ); | ||
|
|
||
| export default Checkbox; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,5 @@ | ||
| import {InputHTMLAttributes, ReactNode} from 'react'; | ||
|
|
||
| export interface CheckboxProps extends InputHTMLAttributes<HTMLInputElement> { | ||
| right?: ReactNode; | ||
| } |
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.
isChecked보다 checked가 더 자연스럽다고 생각해서 is 키워드를 제거하신 걸까요!?
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.
애초에 기본적으로
input태그의 prop으로 있는 이름이checked에요기존에는 input props를 extends하지 않고, isChecked, onChange, labelText 세개의 prop만 받았는데, 이렇게 구현하면 나중에 접근성 태그나 disabled 등 input(Checkbox)를 커스터마이징 하는데 어렵다고 생각해서 input props를 extends 하면서 자연스럽게 isChecked를 input의 기본 prop인 checked로 바꿨습니다
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.
그렇군요. 기본 태그를 활용해서 checked로 변경하는 것 동의합니다.