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

경북대 FE_이은경 4주차 Step2 #81

Open
wants to merge 13 commits into
base: dmsrud1218
Choose a base branch
from

Conversation

dmsrud1218
Copy link

안녕하세요 멘토님!
사실 step1에서 ui 구현에 어려움을 겪는다면 임의로 변경해도 된다고 하셔서 임의로 했었는데 이번은 수정을 하여서 버튼과 수량입력칸 등 조금 더 수정하였습니다!
매번 꼼꼼한 리뷰 감사합니다!

@sjoleee sjoleee changed the base branch from main to dmsrud1218 July 20, 2024 14:26
Copy link

@sjoleee sjoleee left a comment

Choose a reason for hiding this comment

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

고생하셨습니다~! 리뷰 남깁니다.
현재 step1의 내용과 충돌이 발생했는데 한 번 해결해보시면 좋을 것 같습니다

textAlign="center"
width="50px"
/>
<Button onClick={() => handleQuantityChange(quantity + 1)}>+</Button>
Copy link

Choose a reason for hiding this comment

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

최대 개수에 도달하면 비활성화되도록 하는 건 어떨까요?

<Text>카톡 친구가 아니어도 선물 코드로 선물할 수 있어요!</Text>
</Text>
<QuantityWrapper>
<Button onClick={() => handleQuantityChange(quantity - 1)}>-</Button>
Copy link

Choose a reason for hiding this comment

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

최소 개수에 도달하면 비활성화되도록 하는 건 어떨까요?

alert('현금영수증 번호를 입력해주세요!');
return;
}
if (cashReceipt && !/^\d+$/.test(cashReceiptNumber)) {
Copy link

Choose a reason for hiding this comment

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

유효성 검사에서 정규식을 사용할때는 별도의 변수로 사용해주시면 좋을 것 같아요. 정규식만 봐서는 무엇을 위한 건지 파악하기 어려울 때가 많아서요!

Comment on lines +17 to +32
if (message.length === 0) {
alert('카드 메시지를 입력해주세요!');
return;
}
if (message.length > 100) {
alert('카드 메시지는 100자 이내로 입력해주세요!');
return;
}
if (cashReceipt && cashReceiptNumber.length === 0) {
alert('현금영수증 번호를 입력해주세요!');
return;
}
if (cashReceipt && !/^\d+$/.test(cashReceiptNumber)) {
alert('현금 영수증 번호는 숫자만 입력해주세요!');
return;
}
Copy link

Choose a reason for hiding this comment

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

유효성 검사를 하는 함수를 별도로 분리할 수도 있을 것 같네요! 현재는 alert의 메시지가 한글로 작성되어 있어 크게 문제가 될 것 같지는 않지만 많은 검사를 수행해야하거나, 별도의 메시지가 없는 경우에는 각 조건이 어떤 의미를 담는지 파악하기 쉬우면 좋을 것 같아요!

예를 들면, isNotEmptyString(message)isOnlyNumber(cashReceiptNumber) 같은 함수를 만들면 좋지 않을까 싶네요!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants