-
Notifications
You must be signed in to change notification settings - Fork 2
[Refactor] SP4 클래스 관리/마이페이지 QA 반영 #633
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
[Refactor] SP4 클래스 관리/마이페이지 QA 반영 #633
Conversation
|
✅ Storybook 배포 완료! 🔗 https://67e4fd1fd2c7078dceec04a4-hwsqvsgihc.chromatic.com/ |
Deploying dash-client-dev with
|
| Latest commit: |
2d8c1c8
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://a17801ca.dash-client-dev.pages.dev |
| Branch Preview URL: | https://refactor--629-sp4-class-mypa.dash-client-dev.pages.dev |
Deploying dash-client with
|
| Latest commit: |
2d8c1c8
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://6536ea36.dash-client.pages.dev |
| Branch Preview URL: | https://refactor--629-sp4-class-mypa.dash-client.pages.dev |
hansoojeongsj
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.
항상 고생이 많으십니다아 👍 👍
| <section className={styles.containerStyle} onClick={onClick}> | ||
| <div | ||
| <section className={styles.containerStyle}> | ||
| <button |
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.
eslint jsx-a11y 규칙 때문에 div 대신 button으로 변경하신건가여 ??
기존에 ProfileImageUpload 컴포넌트가 있긴 한데, 해당 컴포넌트와 동작 방식이 많이 다른지도 궁금합니다!
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.
eslint jsx-a11y 규칙 때문에 div 대신 button으로 변경하신건가여 ??
맞습니다!
기존에 ProfileImageUpload 컴포넌트가 있긴 한데, 해당 컴포넌트와 동작 방식이 많이 다른지도 궁금합니다!
사용처가 다르긴한데 기능상으로는 비슷해요! 그래서 useImageUploader 공통 hook을 재사용해서 로직을 구현하고 있습니다!
| maxLength={MAX_ACCOUNT_NUMBER_LENGTH} | ||
| onKeyDown={allowOnlyNumberKey} | ||
| onPaste={allowOnlyNumberPaste} | ||
| onChange={(e) => { |
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.
onKeyDown, onPaste에서 이미 숫자만 허용하는 util을 사용하는 것 같은데, onChange에서 한 번 더 값을 확인하는 이유가 React Hook Form에서 accountNumber 값을 확실히 보장하기 위한 용도일까용 ??
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.
약간의 안전장치 느낌일 것 같아요! test는 못했는데 paste(복사)도 keydown(키 입력)도 아닌 경우, 예를 들어 브라우저에서 자동완성 되는 경우는 저 2가지 경우에 안걸릴 수도 있겠다는 생각이 들었어요!
그래서 말씀해주신 것처럼 확실하게 숫자가 되도록 모든 경우를 보장하기 위함인데 한번 테스트 해보면 좋을 것 같아요 👍
📌 Related Issues
✅ 체크 리스트
📄 Tasks
SP4 클래스 관리/마이페이지 QA 반영했습니다.
commit 별로 봐주시면 크게 복잡한 로직은 없어요!
⭐ PR Point
📷 Screenshot
🔔 ETC