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

[김승래] sprint9 #654

Conversation

JustDevRae
Copy link
Collaborator

기본

  • 자유 게시판 페이지 주소는 “/boards” 입니다.
  • 전체 게시글에서 드롭 다운으로 “최신 순” 또는 “좋아요 순”을 선택해서 정렬을 할 수 있습니다.
  • 게시글 목록 조회 api를 사용하여 베스트 게시글, 게시글을 구현합니다.
  • 게시글 title에 검색어가 일부 포함되면 검색이 됩니다.

심화

  • 반응형으로 보여지는 베스트 게시판 개수를 다르게 설정할때 서버에 보내는 pageSize값을 적절하게 설정합니다.
  • next의 prefetch 기능을 사용해봅니다.

주요 변경사항

  • 미션1부터 9까지 Next.js로 적용

멘토에게

  • 미션 10 빼고 모든 페이지를 Next.js로 급히 구현해보느라 코드의 통일성이나 타입 정의가 많이 부족합니다.. 주말에 리팩토링해서 좀 더 깔끔한 코드로 수정할 수 있도록 하겠습니다.
  • 멘토님 말씀에 따라 커밋에 신경을 써보려고 노력했는데 많이 어렵네요.. 좀 더 좋은 커밋 남길 수 있도록 노력해보겠습니다.
  • 셀프 코드 리뷰를 통해 질문 이어가겠습니다.

JustDevRae added 30 commits June 3, 2024 18:04
드롭다운 버튼을 누르면 버튼 밑에 [최신순, 좋아요순] 메뉴가 활성화되고
각 버튼을 누를 시 게시글이 최신순, 좋아요순으로 정렬
검색기능, (최신순, 좋아요순) 정렬기능, additem 페이지 이동 기능
@JustDevRae JustDevRae requested a review from kich555 June 7, 2024 14:26
@JustDevRae JustDevRae added 매운맛🔥 뒤는 없습니다. 그냥 필터 없이 말해주세요. 책임은 제가 집니다. 미완성🫠 죄송합니다.. labels Jun 7, 2024
@kich555
Copy link
Collaborator

kich555 commented Jun 10, 2024

export default function BestArticleSection({ bestArticles }: any) {
  return (...

와 같이 선언과 반환 사이에 아무런 로직이 들어있지 않은 컴포넌트들은 한번쯤 고민해보실 필요가 있을것 같아요
이런 컴포넌트는 어떤 역할을 할까요?
단순히 코드 나누기 그 이상의 역할이 있을까요?
오히려 의미없이 많아진 컴포넌트들 덕분에 가독성이 떨어지진 않을까요?
등등에 대해 한번 고민해보시면 좋을것 같습니다.

컴포넌트를 나누는 이유에는 여러 이유가 있을 수 있습니다.
UI로 나눌수도 있고, 비즈니스 로직으로 나눌 수도 있지만 언제나 그 이유는 명확해야 합니다.
가령 UI를 기준으로 컴포넌트를 나눈다면 해당 컴포넌트는 UI를 처리하는 로직을 명확히 가지고 있어야합니다.
반대로 비즈니스 로직만을 위해 컴포넌트를 나누었다면 해당 컴포넌트도 어떠한 비즈니스 로직을 처리하겠다와 같은 명확한 이유를 가지고 있어야합니다

만약 스스로 생각했을때 명확한 이유가 딱히 보이지 않는다면,
해당 컴포넌트를 나누는 부분에 대해 한번더 생각해보는게 어떨까요? ㅎ

Copy link
Collaborator

@kich555 kich555 left a comment

Choose a reason for hiding this comment

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

고생하셨습니다!!

전반적으로 잘해주셨어요!

다만 타입스크립트를 사용하시면서 any타입을 사용하는건 항상 주의해주셨으면 좋겠습니다!

allArticles,
onSortSelection,
onInputChange,
}: any) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

충분히 타입을 지정할 수 있는 부분같아요!
any 타입사용은 최대한 지양해주세요!

/>
</div>
) : (
<div></div>
Copy link
Collaborator

Choose a reason for hiding this comment

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

그냥 null 하면 안되나요? 빈 div를 두는건 아닌것 같네요 ㅎ

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

넵! 수정하도록 하겠습니다!

Copy link
Collaborator

Choose a reason for hiding this comment

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

이거 검색 입력한것에 대한 처리부분은 어디로 가있나요? Form으로 끝나는 컴포넌트라면 비즈니스 로직은 외부에서 props로 받더라도 검색하기 버튼 정도는 이 컴포넌트에 있어야 할 것 같네요 😅

Copy link
Collaborator

Choose a reason for hiding this comment

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

이 컴포넌트는 설계가 잘못된것 같아요 onChange onSubmit 모두 해당 컴포넌트 내부에서 다루는 특별한 로직이 없네요
SearchForm이라고 컴포넌트 이름을 지었다면 Ui만 가지고 있어야 하는게 아니라 Form에 대한 전반적인 비즈니스 로직도 포함시켜야 할 것 같습니다 ㅎ

Copy link
Collaborator

Choose a reason for hiding this comment

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

이 컴포넌트는 단순히 Props를 받고 tsx를 반환할 뿐인것 같은데 굳이 컴포넌트로 쪼갠 이유가 궁금하네요 ㅎ

컴포넌트 내부가 어떠한 로직도 가지고 있지 않다면 굳이 컴포넌트로 나눠야할지 고민하는게 필요할것 같아요
이렇게 해봤자 불필요하게 파일만 늘리는 것 말고 어떠한 의미가 있는지에 대해 ㅎ

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

나름 깔끔하게 보일려고 분리를 해봤는데 오히려 더 불편했던 것 같습니다.
멘토님 말씀대로 의미없는 컴포넌트 분리하지 않도록 주의하겠습니다!

</button>
</li>
);
})}
Copy link
Collaborator

Choose a reason for hiding this comment

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

   {tags.map((item, idx) => {
              return (
                <li key={idx}>
                  {item}
                  <button onClick={() => handleDeleteTag(item)}>
                    <Image src={Close} alt="닫기" />
                  </button>
                </li>
              );
            })}

이렇게 map의 두번째 파라미터로 인덱스를 가지고있습니다 !

export async function getServerSideProps() {
const besrtArticleResponse = await axios.get(
"articles/?pageSize=3&orderBy=like"
);
Copy link
Collaborator

Choose a reason for hiding this comment

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

  const besrtArticleResponse = await axios.get("articles", {
    params: {
       pageSize: 3,
       orderBy:"like"
    }
  });

axios를 사용하면 이렇게 searchParam등을 객체로 관리할 수 있어요!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

넵! 코드에 적용시켜 보겠습니다!

const besrtArticleResponse = await axios.get(
"articles/?pageSize=3&orderBy=like"
);
const allArticleResponse = await axios.get("articles/?orderBy=like");
Copy link
Collaborator

Choose a reason for hiding this comment

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

그리고 "articles/?orderBy=like" => "articles?orderBy=like" 이렇게..!

import Back from "@/assets/images/icons/ic_back.svg";
import { useEffect, useState } from "react";

export async function getServerSideProps(context: any) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

이것도 any타입 말고 타입 충분히 지정해줄 수 있어요!!!

any타입을 사용하는 부분이 있을때마다 타입 추론이 무너져서 타입스크립트를 사용하는 의미가 점점 사라집니다!

const productResponse = await axios.get(`/products/${productId}`);
const commentsResponse = await axios.get(
`/products/${productId}/comments?limit=3`
);
Copy link
Collaborator

Choose a reason for hiding this comment

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

위와같이 async함수에 여러개의 await구문이 들어간다면 해당 함수는 await구문마다 해당 비동기 함수가 끝날때까지 기다리게 됩니다.
이는 async함수 전체의 waterfall을 초래하게 되는데요
이는 모든 await함수의 결과가 나와야 async함수가 끝나기 때문에 성능적으로 좋지 않습니다.
모든 비동기 함수가 병렬적으로 실행될 수 있게 하려면 Promise.all을 활용해보시는걸 추천드립니다 ㅎ
Promise.all
Promise.allSettled

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

넵! 코드에 활용해보겠습니다!!

@kich555 kich555 merged commit c037f49 into codeit-bootcamp-frontend:Next.js-김승래 Jun 10, 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