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

[김수영] sprint8 #112

Conversation

swim-kim
Copy link
Collaborator

요구사항

기본

  • 스프린트 미션 1 ~ 7에 대해 typescript를 적용해주세요

심화

  • any타입을 최소한으로 써주세요

멘토에게

  • 자식컴포넌트에서 쓰이는 데이터들의 타입도 부모에서 다 정의해주는게 맞을까요?? 일단은 다 정의해줬는데 중복되는 코드가 여러번 쓰이는것 같고, 선별적으로 쓰려다보니 오류가 나는 부분이 계속 생겨서 일단 오류가 나지 않는 방향으로만 코드를 짰는데 더 나은 방식이 있는지 궁금합니다

@swim-kim swim-kim self-assigned this Oct 19, 2024
@swim-kim swim-kim added the 매운맛🔥 뒤는 없습니다. 그냥 필터 없이 말해주세요. 책임은 제가 집니다. label Oct 19, 2024
Copy link
Collaborator

@GANGYIKIM GANGYIKIM left a comment

Choose a reason for hiding this comment

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

수영님 이번 스프린트 미션 고생하셨습니다~
다 잘 작성하셨는데 반복되는 타입이 좀 있는 것 같아요~
멘토에게 섹션에 말씀해주신 것처럼 타입을 그때그때 다시 선언해주면 유지보수도 어렵고 코드의 가독성도 떨어집니다.
저희가 공통 컴포넌트를 만들듯이 자주 사용되는 타입을 정의해두시고 이를 export 해서 다른곳에서 같이 쓰세요~

Comment on lines +6 to +12
const allowedParams = ["orderBy", "pageSize", "page", "keyword"];
const invalidParams = Object.keys(params)
.filter(key => !allowedParams.includes(key));

if (invalidParams.length > 0) {
console.error(`Invalid parameters detected: ${invalidParams.join(", ")}`);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

P3:
👍

}

try {
const response = await axios.get(`${BASE_URL}/products`, {
Copy link
Collaborator

Choose a reason for hiding this comment

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

P1:
가능하면 어떤 값이 return될지 아래처럼 작성해주세요~

Suggested change
const response = await axios.get(`${BASE_URL}/products`, {
const response = await axios.get<ProductProps>(`${BASE_URL}/products`, {

Comment on lines +52 to +55
params: {
limit: params.limit,
cursor: params.cursor
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

P3:
아래 두 경우도 동일동작합니다~

Suggested change
params: {
limit: params.limit,
cursor: params.cursor
}
params
Suggested change
params: {
limit: params.limit,
cursor: params.cursor
}
params: { ...params }

@@ -0,0 +1,64 @@
import React from 'react';
import './ProductInfo.css';
import profileimg from '../../../../assets/images/profile.png';
Copy link
Collaborator

Choose a reason for hiding this comment

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

P3:

Suggested change
import profileimg from '../../../../assets/images/profile.png';
import profileImg from '../../../../assets/images/profile.png';

</div>
<div className='like-box'>
<div>
<img className='hearticon' src={heartIcon} alt='좋아요 아이콘' />
Copy link
Collaborator

Choose a reason for hiding this comment

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

P3:

Suggested change
<img className='hearticon' src={heartIcon} alt='좋아요 아이콘' />
<img className='heart-icon' src={heartIcon} alt='좋아요 아이콘' />


function AllItem() {
const [itemList, setItemList] = useState<ItemList>({ totalCount: 0, list: [] });
const [pageSize, setPageSize] = useState(getPageSize());
Copy link
Collaborator

Choose a reason for hiding this comment

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

P3:
아래처럼 타입을 명시해주시면 number가 아니라 더 정확한 타입으로 좁힐 수 있습니다.

Suggested change
const [pageSize, setPageSize] = useState(getPageSize());
const [pageSize, setPageSize] = useState<ReturnType<typeof getPageSize>>(getPageSize());

function AllItem() {
const [itemList, setItemList] = useState<ItemList>({ totalCount: 0, list: [] });
const [pageSize, setPageSize] = useState(getPageSize());
const [order, setOrder] = useState('recent');
Copy link
Collaborator

Choose a reason for hiding this comment

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

P1:
order 타입이 string이 아니라 'recent' | 'favorite' 일 수 잇도록 해주시면 더 좋을 것 같아요.

{order === 'recent' ? '최신순' : '인기순'} <span className={styles.dropdownArrow}>▼</span>
</button>
<ul className={`${styles.dropdownMenu} ${isDropdownOpen ? styles.show : ''}`}>
<li className={styles.dropdownOption} onClick={handleNewestClick}>최신순</li>
Copy link
Collaborator

Choose a reason for hiding this comment

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

P1:
li 태그보다 button 같은 것을 사용해주세요~
이렇게 해주시면 handleNewestClick, handleFavoriteClick 가 아니라 하나의 함수로 사용할 수 있습니다.

Suggested change
<li className={styles.dropdownOption} onClick={handleNewestClick}>최신순</li>
<li className={styles.dropdownOption}>
<button type='button' value='recent' onClick={handleNewestClick}>최신순</button>
</li>

Comment on lines +6 to +17
interface Item {
id: number;
name: string;
description: string;
price: number;
tags: string[];
images: string[];
ownerId: number;
ownerNickname: string;
favoriteCount: number;
createdAt: string;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

P1:
BestItem 컴포넌트에서도 동일한 인터페이스가 사용되니 하나를 정의해서 같이 쓰면 좋을 것 같습니다.

const { productId } = useParams();
const [item, setItem] = useState(null);
const { productId } = useParams<{ productId :string}>();
const [item, setItem] = useState<Item | undefined>(undefined);
Copy link
Collaborator

Choose a reason for hiding this comment

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

P3:
아래 코드도 동일동작합니다~

Suggested change
const [item, setItem] = useState<Item | undefined>(undefined);
const [item, setItem] = useState<Item>();

@GANGYIKIM GANGYIKIM merged commit b73df2a into codeit-bootcamp-frontend:React-김수영 Oct 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
매운맛🔥 뒤는 없습니다. 그냥 필터 없이 말해주세요. 책임은 제가 집니다.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants