[장바구니 미션 Step 1] 야미(이다인) 미션 제출합니다. #168
Conversation
Co-authored-by: 박정규 <[email protected]>
Co-authored-by: 박정규 <[email protected]>
Co-authored-by: 박정규 <[email protected]>
styled-components를 최신 버전으로 설치하여 에러 해결 (@latest)
+ 상수화, 개행 수정
|
빠른 리뷰 감사합니다 리트! 최대한 리뷰 반영하였습니다.
📢 오류가 생길 수 있으니 장바구니 앱/스토리 북 사용 전 로컬스토리지를 비워주세요!
❓궁금한 점1)
html만으로 해결하는 방법이 아닌 자바스크립트를 이용해서라도 2)export const QUANTITY: Readonly<Record<string, string>> = {
INITIAL: '1',
NONE: '0',
};위의 경우 string으로 타입을 지정해주는 게 좋을지, 두 개니까 유니온으로 타입을 더 좁혀주는 게 좋을지 모르겠습니다. 3)웹 접근성을 위해 svg 버튼(svg를 버튼으로 이용하는 경우)은 항상 버튼 태그로 감싸줘야 하나요? 아니면 버튼 태그로 감싸주는 것 외에 접근성을 높이는 다른 방법이 있나요? 4)모바일 사파리에서는 5)스토리북에서 그냥 QuantityInput을 불러와 컴포넌트로 사용하면 ✅ 피드백 반영[9bb45d1] Readonly로 불필요하게 타입 지정한 부분 수정 👇🏻추가 커밋도 확인해주세요~! |
유효하지 않은 값은 localStorage에 저장되지 않도록 함
There was a problem hiding this comment.
야미, 렌더링 최적화까지 챙기셨군요. 대단합니다👍
Approve 할게요!
웹 접근성을 위해 svg 버튼(svg를 버튼으로 이용하는 경우)은 항상 버튼 태그로 감싸줘야 하나요? 아니면 버튼 태그로 감싸주는 것 외에 접근성을 높이는 다른 방법이 있나요?
+) 웹 접근성 공부를 위해 좋은 문서/자료가 있다면 추천 부탁드립니다!
버튼 태그로 감싸는 게 확실해요.
https://codepen.io/Nice2MeatU/pen/dqmypX
찾아보니까 다른 방법도 있는 거 같은데요.
svg라는 태그의 본래 역할이 있기도 하니 권장하고 싶지 않네요😅
https://stackoverflow.com/questions/36902118/do-i-need-do-wrap-an-svg-with-button-to-indicate-it-is-a-button
'웹접근성 검사 도구'를 직접 찾아보시고 검사해봐도 좋겠네요.
| const validateResponse = async (response: Response) => { | ||
| if (response.ok) { | ||
| const data = await response.json(); | ||
|
|
||
| return data; | ||
| } else { | ||
| throw new Error(`[HTTP error] status: ${response.status}`); | ||
| } | ||
| }; |
There was a problem hiding this comment.
fetchData 유틸은 바로 사용할 수 있게 만들어주신 거 좋아요.
저는 validateResponse 메서드가 data를 반환하고 있는 부분을 말씀드렸던 겁니다.
payload 추출이 주요 의도라는 생각이 들었어요. 검증은 부기능이고요.
이름을 바꾸면 어떨까요? extractPayload를 제안해봅니다.
| const validateResponse = async (response: Response) => { | ||
| if (response.ok) { | ||
| const data = await response.json(); | ||
|
|
||
| return data; | ||
| } else { | ||
| throw new Error(`[HTTP error] status: ${response.status}`); | ||
| } | ||
| }; |
There was a problem hiding this comment.
fetchData라는 유틸이 굉장히 low한 수준의 코드라고 생각하는데요.
에러 처리를 강제로 그냥 console.log만 찍고 있어요.
이 부분은 좀 더 고민해보는 게 좋겠습니다.
이렇게 작성하면 fetchData를 호출한 코드에서는
어떤 에러가 발생했는 지 알 수 없어요
| export const getDataFromLocalStorage = (key: string) => { | ||
| return localStorage.getItem(key); | ||
| }; |
There was a problem hiding this comment.
localStorage.getItem을 그대로 쓰는 것과 차이가 없어요.
getter에도 반환 타입이 있으면 좋겠군요
| const handleDotPrevent: KeyboardEventHandler<HTMLInputElement> = (event) => { | ||
| if (event.key === '.') event.preventDefault(); | ||
| }; | ||
|
|
||
| return ( | ||
| <S.Wrapper> | ||
| <Input | ||
| type="number" | ||
| value={value} | ||
| id={id} | ||
| inputMode="numeric" | ||
| name="quantity" | ||
| aria-label="quantity-input" | ||
| autoComplete="on" | ||
| autoFocus | ||
| min={0} | ||
| max={99} | ||
| css={QuantityInputStyle} | ||
| onWheel={handleScrollPrevent} | ||
| onChange={onChange} | ||
| onKeyDown={handleDotPrevent} | ||
| /> |
There was a problem hiding this comment.
| const handleDotPrevent: KeyboardEventHandler<HTMLInputElement> = (event) => { | |
| if (event.key === '.') event.preventDefault(); | |
| }; | |
| return ( | |
| <S.Wrapper> | |
| <Input | |
| type="number" | |
| value={value} | |
| id={id} | |
| inputMode="numeric" | |
| name="quantity" | |
| aria-label="quantity-input" | |
| autoComplete="on" | |
| autoFocus | |
| min={0} | |
| max={99} | |
| css={QuantityInputStyle} | |
| onWheel={handleScrollPrevent} | |
| onChange={onChange} | |
| onKeyDown={handleDotPrevent} | |
| /> | |
| const handleDigitOnlyAllow: FormEventHandler<HTMLInputElement> = ( | |
| event: FormEvent<HTMLInputElement> | |
| ) => { | |
| const input = event.currentTarget; | |
| var newValue = input.value.replace(/[^0-9]+/g, ""); | |
| // number 타입은 커서 위치를 바꾸는 법을 모르겠네요. | |
| // 커서 위치를 유지하기 위해 값을 지우고 다시 할당. | |
| input.value = ""; | |
| input.value = newValue; | |
| }; | |
| return ( | |
| <S.Wrapper> | |
| <Input | |
| type="number" | |
| value={value} | |
| id={id} | |
| inputMode="numeric" | |
| name="quantity" | |
| aria-label="quantity-input" | |
| autoComplete="on" | |
| autoFocus | |
| min={0} | |
| max={99} | |
| css={QuantityInputStyle} | |
| onWheel={handleScrollPrevent} | |
| onChange={onChange} | |
| onInput={handleDigitOnlyAllow} | |
| /> | |
숫자만 입력할 수 있도록 바꿔봤습니다.
There was a problem hiding this comment.
모바일 사파리에서는 input type=number의 스핀 버튼이 지원되지 않는지 증감 버튼이 클릭되지 않는 것을 발견했습니다. 그래서 모바일은 증감 버튼을 없앨까(입력만 가능하게) 고민중인데, 한 브라우저(모바일 사파리)에서 지원되지 않는다는 이유만으로 기능을 없애는 것이 맞는지 잘 모르겠습니다. 이럴 때 리트는 기능을 없애시나요? 그냥 두시나요? 아니면 추가적으로 기능을 만들어 모바일 사파리에서도 버튼을 사용할 수 있도록 하시나요? ( 추가적으로 기능을 만들면 input type=number를 쓰는 이유가 없어져서 없앨까 말까를 고민했습니다 😂 )
여러 브라우져를 지원하기 위함이니까 실무 프로젝트였다면 자체 구현을 했을겁니다😂
하지만, 이번 미션에서 주요 사항은 아니라고 생각해서 known issue로 두고 다른 부분에 집중하면 어떨까요?
+혹시 나중에 자체 구현하시더라도 input type number은 모바일 가상키보드 형태에 영향을 주니까 유지해도 될 거 같아요!
| export const QUANTITY: Readonly<Record<string, string>> = { | ||
| INITIAL: '1', | ||
| NONE: '0', | ||
| }; |
There was a problem hiding this comment.
위의 경우 string으로 타입을 지정해주는 게 좋을지, 두 개니까 유니온으로 타입을 더 좁혀주는 게 좋을지 모르겠습니다.
유니온으로 지정해주면 속성이 추가될 경우 타입도 늘어나니까 불편할 것 같다는 생각도 들고
타입은 최대한 좁히는 게 좋지 않을까 라는 생각도 들어서요. 리트의 생각은 어떤가요??
특수한 수량을 나타내기 위한 상수죠.
이후에도 속성이 늘어날 수 있다고 생각하면 지금 상태를 유지하는 게 좋다고 생각해요
| }); | ||
| }); | ||
|
|
||
| await new Promise((resolve) => setTimeout(resolve, 2000)); |
There was a problem hiding this comment.
자주 쓰이고 있으니 waitFor 함수를 활용해서 util 함수로 추출해도 좋을 거 같아요
There was a problem hiding this comment.
waitFor를 활용하여 util로 빼보려고 했으나 계속 run time out 에러가 나서 실패했습니다🥲
우선 위 코드를 함수로 빼서 사용했는데 util화는
다음 step에서 다시 적용해보도록 하겠습니다!
There was a problem hiding this comment.
페이지 동작을 자동으로 테스트해주니까 좋네요 👍
스토리북 전반적으로 docs에 설명이 자세히 적어주셔서 컴포넌트의 세부사항을 파악하기 좋았습니다
안녕하세요 리트! 야미입니다. 처음으로
recoil써봤는데 쉽지 않네요...😂recoilFamily... 등 다양한 기능을 들었지만, 적용은 하지 못했습니다...
제가 알지 못하는 recoil 기능 중 현재 로직에서 유용하게 쓰일 만한 것이 있다면 추천 부탁드립니다!
그리고 사용성이나 로직 개선 방향 등에 대해서 편하게 피드백 부탁드립니다 😄
📢 오류가 생길 수 있으니 로컬스토리지를 비워주세요!
🥲 아쉬운 점
❓궁금한 점
type=number일 때 한글 입력/점(./삼성 모바일에서)을 막는 방법이 있나요? ( value로 값은 들어가지 않지만, 화면상 input에 한글이 보이는 오류) 찾아보니 inputtype=number의 자체적인 오류라고 하던데 정말 방법이 없는 것인지 궁금합니다.