-
Notifications
You must be signed in to change notification settings - Fork 37
[이석찬] sprint9 #310
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
[이석찬] sprint9 #310
The head ref may contain hidden characters: "Next-\uC774\uC11D\uCC2C-sprint9"
Conversation
|
PR을 올리고 보니 assets 디렉토리가 pages 안에 있어, 다음 미션에는 public 디렉토리로 옮겨서 올리도록 하겠습니다. |
Lanace
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.
개발하시느라 시간 많이 드셨을것같은데 고생 많으셨어요ㅠ
아마 반복되는 코드들이 많이 있다고 느끼셨을텐데, 그런 부분들이 처음에 많이 드는게 정상이긴 해요ㅎㅎ
오히려 그런 생각이 드는게 좋은거기도 하구요...!
비슷한 코드가 나오면 어떻게 공통화 하는게 좋을지, 실제로 공통화 했을때 좋았는지를 생각해보면서 개발하시다보면 분명 더 좋은 코드가 되어있을꺼에요~!!
고생 많으셨습니다ㅎㅎ
| orderBy: "recent" | "like" = "recent", | ||
| keyword: string = "" | ||
| ): Promise<ArticleListResponse> { | ||
| const url = `https://panda-market-api.vercel.app/articles?page=${page}&pageSize=${pageSize}&orderBy=${orderBy}&keyword=${keyword}`; |
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.
이렇게되면 가독성이 좋진 않은것같아요ㅠ
그리고 서버에 따라서는 keyword가 처럼 공백문자인 경우에 이상한 값을 줄수있어서 optional이라면 아에 안보내는게 조금더 좋아요ㅎㅎ;;
그래서 이런식으로 하면 어떨까요?
나중에 추가되는 값을 바로바로 볼 수 있고, 다루기도 쉬울것같아서요!
const url = new URL("https://panda-market-api.vercel.app/articles");
url.searchParams.append("page", page.toString());
url.searchParams.append("pageSize", pageSize.toString());
url.searchParams.append("orderBy", orderBy);
url.searchParams.append("keyword", keyword);| } catch (error) { | ||
| console.error("Error fetching articles:", error); | ||
| return { | ||
| totalCount: 0, | ||
| list: [], | ||
| }; | ||
| } |
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.
차라리 실패했을땐 error를 throw 하는게 좀더 좋을 수 있어요ㅎㅎ
throw new Error(error);| images: { | ||
| domains: ["sprint-fe-project.s3.ap-northeast-2.amazonaws.com"], // 허용할 도메인 추가 | ||
| }, |
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.
👍
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.
사용하지 않는 파일은 아에 삭제하는게 좋을것같아요ㅎㅎ!
| export default function Document() { | ||
| return ( | ||
| <Html lang="en"> | ||
| <Html lang="ko"> |
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.
꼼꼼하시네요ㅎㅎ 👍
| <AddItemForm | ||
| label="판매가격" | ||
| id="price" | ||
| type="text" | ||
| placeholder="판매가격을 입력해주세요" | ||
| value={price} | ||
| onChange={handleChange} | ||
| /> |
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.
가격도 물론 string 으로 처리할 순 있긴 하겠지만 가능하면 number로 처리하는게 좋지 않을까여?ㅎㅎ
| interface AddItemFormProps { | ||
| label: string; | ||
| type?: string; | ||
| placeholder: string; | ||
| id: string; | ||
| value: string; | ||
| onChange: ( | ||
| event: React.ChangeEvent<HTMLInputElement | HTMLTextAreaElement> | ||
| ) => void; | ||
| isTextArea?: boolean; | ||
| onKeyDown?: ( | ||
| event: React.KeyboardEvent<HTMLInputElement | HTMLTextAreaElement> | ||
| ) => void; | ||
| } |
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.
너무 범용으로 만들려고 하신건 아닐까 싶긴 하네여ㅠ
textarea도 되고, input도 되게 만들려고 하다보니 너무 많은 타입들이 생기게 되었는데, 차라리 나눠두는게 사용하긴 더 편하실꺼에요~
| if (loading) return <p>Loading...</p>; | ||
| if (error) return <p>Error: {error}</p>; | ||
|
|
||
| return ( |
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.
예외처리들 다 잘 처리하셨네요ㅎㅎ!
| try { | ||
| const data: Item[] = await getItems(); | ||
| setItems(data); | ||
| setLoading(false); | ||
| } catch (err: any) { | ||
| setError(err.message); | ||
| setLoading(false); | ||
| } |
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.
만약 처음 요청할때 error 가 발생해서 setError 에 들어왔고,
다음번 요청할떄 성공하면 계속 error가 설정되어있어서 에러화면이 보일것같아요ㅠ
try쪽으로 들어오기전에 setError 로 에러를 초기화 해줘야할것같은데여...?
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.
setLoading 도 같이 초기화 해야겠네요!
| const sortedItems = items | ||
| .sort((a, b) => b.favoriteCount - a.favoriteCount) | ||
| .slice(0, 4); |
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.
이런 값은 useMemo를 이용해보시면 좀더 성능이 좋은 컴포넌트를 만드실 수 있을거에요~!
요구사항
기본
심화
주요 변경사항
스크린샷
멘토에게