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주차 과제 step3,4 #115

Open
wants to merge 24 commits into
base: rudtj
Choose a base branch
from

Conversation

rudtj
Copy link

@rudtj rudtj commented Jul 21, 2024

안녕하세요 멘토님
4주차 과제 step3,4 제출합니당
이번에 react hook form을 처음 사용해봐서 이렇게 사용하는 게 맞는지 잘 모르겠지만 열심히 구현하는 것에 초점을 두었습니다!
이번 주도 너무 고생 많으셨고 항상 꼼꼼한 피드백 감사합니다~!!

@JunilHwang JunilHwang self-requested a review July 21, 2024 07:41
Copy link

@JunilHwang JunilHwang left a comment

Choose a reason for hiding this comment

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

안녕하세요 경서님~
Step 3 ~ 4 잘 진행해주셨네요.
마지막 단계라서 피드백 반영을 위해 일단 Request Changes 로 남겨놓겠습니다!

Comment on lines +16 to +19
const BASE_URL = 'https://kakao-tech-campus-mock-server.vercel.app/api/v1/products/';

const getProductDetail = async (productId: string) => {
const res = await axios.get<{ detail: ProductDetailData }>(`${BASE_URL}${productId}/detail`);

Choose a reason for hiding this comment

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

aixosInstance, axios.create 등을 활용해보세요!

https://axios-http.com/docs/instance

Comment on lines +25 to +28
const BASE_URL = 'https://kakao-tech-campus-mock-server.vercel.app/api/v1/products/';

export const getProductOption = async (productId: string) => {
const res = await axios.get<ProductOptionData>(
`https://kakao-tech-campus-mock-server.vercel.app/api/v1/products/${productId}/detail`,
);
const res = await axios.get<ProductOptionData>(`${BASE_URL}${productId}/detail`);

Choose a reason for hiding this comment

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

앞선 내용을 참고해주세요!

Comment on lines +24 to 26
const { data: productDetail } = useGetProductDetail(productId ?? '');
const { data: productOption, isLoading: isOptionLoading } = useGetProductOption(productId ?? '');
const [isLoading, setIsLoading] = useState(true);

Choose a reason for hiding this comment

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

이 친구들을 묶어서 사용하는 훅도 만들 수 있지 않을까요?

Comment on lines 34 to 51
useEffect(() => {
const fetchProductDetail = async () => {
try {
console.log('ID:', productId);
const res = await axios.get(
`https://kakao-tech-campus-mock-server.vercel.app/api/v1/products/${productId}/detail`,
);
if (!res.data.detail) {
navigate(RouterPath.notFound);
return;
}
setProductDetail(res.data.detail);
setIsLoading(false);
} catch (error) {
console.error(error);
} finally {
setIsLoading(false);
}
};
fetchProductDetail();
}, [productId, navigate]);

Choose a reason for hiding this comment

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

이 코드는 이제 불필요하지 않나요?

@@ -78,15 +78,18 @@ export const DetailPage = () => {
}
};

if (isLoading || !productDetail) {
if (isLoading || !productDetail || !productOption) {

Choose a reason for hiding this comment

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

const { loading, detail, options } = useProductDetailWithOptions();
앞에서 언급한 커스텀 훅을 사용해서 이 부분을 loading 하나로 묶어버리를 수 있을 것 같아요!

value={productCount}
onChange={handleProductCountChange}
{...register('productCount', { min: 1 })}
onChange={(e) => handleProductCountChange(parseInt(e.target.value))}

Choose a reason for hiding this comment

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

changeProductCount 처럼 이 함수를 표현해야 더 적합할 것 같아요.

handleProductCountChange라는 이름은 결국 onProductCountChange 라는 이벤트를 처리한다는 이야기인데
이 페이지에서 onProductCountChange 가 발생하는 순간이 언제일까요?

아마 이벤트를 표현하려면 이렇게 해야 적합하지 않을까요?

const handleProductCountChange = () => console.log('productCount 가 변경되었음을 알리는 이벤트입니다.');

const changeProductCount = (value: number) => {
  setValue('productCount', value);
  handleProductCountChange();
};

이런식으로 이벤트와 함수를 명확히 구준해주세요!

Comment on lines +68 to +83
if (!message.trim()) {
alert('메시지를 입력해주세요.');
return;
}
if (message.length > 100) {
alert('메시지를 100자 이내로 입력해주세요.');
return;
}
if (receiptRequested) {
if (!receiptNumber.trim()) {
alert('현금 영수증 번호를 입력해주세요.');
return;
}
if (isNaN(Number(receiptNumber))) {
alert('현금 영수증 번호를 숫자만 입력해주세요.');
return;

Choose a reason for hiding this comment

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

이쪽 코드도 선언형으로 변경할 수 있을 것 같아요!

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