-
Notifications
You must be signed in to change notification settings - Fork 47
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
충남대 FE_강병현 4주차 과제 Step2 #89
base: kang-kibong
Are you sure you want to change the base?
충남대 FE_강병현 4주차 과제 Step2 #89
Conversation
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.
고생하셨습니다. 피드백 드려요.
hasCashRecipt: boolean; | ||
cashReciptType: string; | ||
cashReciptNumber: string; |
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.
hasCashRecipt: boolean; | |
cashReciptType: string; | |
cashReciptNumber: string; | |
hasCashReceipt: boolean; | |
cashReceiptType: string; | |
cashReceiptNumber: string; |
오타 하나 피드백 드려요.
<ProductOrderContainer> | ||
<QuantitySelectorConatiner> | ||
<Title>{name}</Title> | ||
<QuantitySelector giftOrderLimit={giftOrderLimit} onSetCount={setCount} count={count} /> |
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.
<QuantitySelector giftOrderLimit={giftOrderLimit} onSetCount={setCount} count={count} /> | |
<QuantitySelector giftOrderLimit={giftOrderLimit} onSetCount={setCount} /> |
count는 존재하지 않는 속성이라고 타입 에러가 발생하고 있네요.
max-width: 360px; | ||
`; | ||
|
||
const QuantitySelectorConatiner = styled.div` |
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.
const QuantitySelectorConatiner = styled.div` | |
const QuantitySelectorContainer = styled.div` |
오타 하나 피드백드려요. VSCode 쓰신다면 Code Spell Checker 추천드려요.
mb={4} | ||
value={cashReciptNumber} | ||
onChange={(e) => onInputChange('cashReciptNumber', e.target.value)} | ||
/> |
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.
현금영수증 신청에 체크를 했을 때만 select와 input이 나오도록 조건부 렌더링하는게 좋겠습니다. 지금은 체크하지 않았는데 값을 입력받을 수 해둬서 사용자가 보고 판단을 해야할 거라서요.
}: PaymentProps) { | ||
const handleClick = () => { | ||
const errorMessage = validatePayment(message, hasCashRecipt, cashReciptNumber); | ||
if (errorMessage) return alert(errorMessage); |
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.
UX 관점에서 1가지 생각해볼만한 점을 적어둘게요. 값을 입력하는 도중에 에러를 보여주거나, 값을 입력하고 나서 포커스가 다른 곳으로 옮겨졌을 때 에러를 보여주는 패턴이 보통 일반적으로 잘 활용되는 UX이긴 합니다. 제출하기 전에 사용자가 했던 행동에 대해 빠른 시점에 피드백을 받을 수 있으니까요. 그래서 제출할 때 유효성 에러를 안내하는 것과 alert로 보여주는건 사용자 경험상 좋은 경험은 아닐 수 있어요.
onInputChange, | ||
}: ReceiptFormProps) { | ||
return ( | ||
<form> |
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 요소에서 입력하다가 enter를 누르게 되면 페이지가 새로고침 되면서 현재까지 입력했던 값이 유실되네요. form 제출은 button click을 통해서만 이뤄지기보다는 가급적이면 form submit event를 통해서 처리되는게 마우스, 키보드 접근성을 위해 권장됩니다.
#74 병합 후 충돌 발생하고 있어서 해결되면 병합하도록 하겠습니다. |
안녕하세요. 카테캠 프론트엔드 파트 수강 중인 충남대학교 강병현입니다!
초반에 고민을 많이 한 탓에 step2까지 밖에 진행을 하지 못하였습니다..
내일 step4까지 마무리하여 PR날리도록 하겠습니다. 죄송합니다. 🙇♂️