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

[장준혁] Sprint10 #628

Conversation

CitrusSoda
Copy link
Collaborator

요구사항

기본

상품 등록 페이지

  • 상품 등록 페이지 주소는 “/addboard” 입니다.
  • 게시판 이미지는 최대 한개 업로드가 가능합니다.
  • 각 input의 placeholder 값을 정확히 입력해주세요.
  • 이미지를 제외하고 input 에 모든 값을 입력하면 ‘등록' 버튼이 활성화 됩니다.
  • 회원가입, 로그인 api를 사용하여 받은accessToken을 사용하여 게시물 등록을 합니다.
  • ‘등록’ 버튼을 누르면 게시물 상세 페이지로 이동합니다.

상품 상세 페이지

  • 상품 상세 페이지 주소는 “/addboard/{id}” 입니다.
  • 댓글 input 값을 입력하면 ‘등록' 버튼이 활성화 됩니다.
  • 활성화된 ‘등록' 버튼을 누르면 댓글이 등록됩니다

주요 변경사항

  • react-hook-form을 이용하여 form 관리를 하였습니다.
  • 게시글은 ISR, 댓글은 CSR로 구현하였습니다.

스크린샷

스프린트 미션 10 배포

image

image

멘토에게

  • 게시글 같은 경우 처음에 SSG를 이용하여 부르고, 그 후 유저 인터랙션이 있으면(ex. 드롭박스를 클릭하여 정렬 순서 변경, 페이지네이션 등) CSR로 데이터를 가져오는 것이 괜찮은 방법일까요?
  • 셀프 코드 리뷰를 통해 질문 이어가겠습니다.

@CitrusSoda CitrusSoda requested a review from 1005hoon June 7, 2024 09:06
@CitrusSoda CitrusSoda self-assigned this Jun 7, 2024
@CitrusSoda CitrusSoda added the 매운맛🔥 뒤는 없습니다. 그냥 필터 없이 말해주세요. 책임은 제가 집니다. label Jun 7, 2024
@CitrusSoda CitrusSoda changed the title Next.js 장준혁 sprint10 [장준혁] Sprint10 Jun 7, 2024
@1005hoon
Copy link
Collaborator

@CitrusSoda

준혁님! 먼저 질문 답변드릴게요 ㅎㅎ

Q. 게시글 같은 경우 처음에 SSG를 이용하여 부르고, 그 후 유저 인터랙션이 있으면(ex. 드롭박스를 클릭하여 정렬 순서 변경, 페이지네이션 등) CSR로 데이터를 가져오는 것이 괜찮은 방법일까요?
이 경우, ssg 로 초반 데이터를 가져와 사용하고, 이 데이터를 기반해 인터랙션을 제공한다는 방법으로 제가 이해를 했는데요. 물론 이렇게 처리해도 된답니다. 다만 우려가 되는 부분이 하나 있어요. NextJS에서 제작한 스태틱 페이지가 만약 데이터베이스에 저장된 데이터를 캐싱하고 있거나, revalidate 텀이 존재하고 있다면 우리가 프론트단에서 UI interaction을 통해 서버쪽 데이터를 변경한다 하더라도, ssg에서는 이를 반영하지 않는 경우가 발생할 수 있습니다. 따라서 이에 대해서는 client side data mutation이 server side data revalidation까지 될 수 있도록 조치를 해 주어야 할 거에요!

Copy link
Collaborator

@1005hoon 1005hoon left a comment

Choose a reason for hiding this comment

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

@CitrusSoda

저번에 드렸던 피드백을 최대한 반영해주신게 보여요 ㅎㅎ
이번에는 조금 더 모듈화가 필요한 이유를 중점으로 리뷰를 드렸는데요, 한번 참고 부탁드릴게요.
고생하셨습니다!

Comment on lines +40 to 43
const res = await axios.get(
`/articles?pageSize=${itemsPerRow}&orderBy=like`,
);
const boards = res.data.list ?? [];
Copy link
Collaborator

Choose a reason for hiding this comment

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

@CitrusSoda

좋습니다!
그럼 이제 UI / Container에서는 데이터를 불러오는 사이클만 관리하도록 하고
axios를 사용한 네트워크 통신과, 발생한 오류 그리고 데이터 후처리등은 서비스 레이어로 한번 분리시켜볼까요?

Comment on lines +76 to +88
<Dropdown>
<DropdownToggle>
<p>{boardOrder === 'recent' ? '최신순' : '좋아요순'}</p>
<Image src={arrowDownIcon} alt="down arrow icon" />
</div>
{/* 드롭다운 */}
{isOpen && (
<div
ref={dropDownRef}
className="absolute right-0 top-12 z-50 flex w-32 flex-col rounded-xl border bg-white sm:w-full"
>
<button
onClick={() => setBoardOrder('recent')}
className="h-[42px]"
>
최신순
</button>
<hr />
<button
onClick={() => setBoardOrder('like')}
className="h-[42px]"
>
좋아요순
</button>
</div>
)}
</div>
</DropdownToggle>
<DropdownMenu>
<DropdownItem onClick={() => setBoardOrder('recent')}>
최신순
</DropdownItem>
<DropdownItem onClick={() => setBoardOrder('like')}>
좋아요순
</DropdownItem>
</DropdownMenu>
</Dropdown>
Copy link
Collaborator

Choose a reason for hiding this comment

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

@CitrusSoda

오 좋은데요?
이제 여기서 조금 더 관심사를 분리해서 Dropdown이라는 공통 UI 콤포넌트를 우리 보드에서만 사용되는 콤포넌트로 만들어 boardOrder에 따른 라벨링이나 로직들을 모듈화 해보면 어떨까요?

예를 들어, board-sorting-operation.tsx 와 같은 콤포넌트를 만들고, 그 안에서 우리가 구현한 드롭다운이 조립되도록 하는거죠.

이를 통해 <p>{boardOrder === 'recent' ? '최신순' : '좋아요순'}</p> 이런 내용이나

   <DropdownItem onClick={() => setBoardOrder('recent')}>
                최신순
              </DropdownItem>
              <DropdownItem onClick={() => setBoardOrder('like')}>
                좋아요순
              </DropdownItem>

와 같이 보드라는 콤포넌트의 핵심적인 요소가 아닌 내용을 조금 더 모듈화 시켜 분리시킴으로 가독성을 더 향상시킬 수 있어 보여요

Comment on lines +96 to +127
<Link href={`/boards/${board.id}`}>
<div className="flex h-40 flex-col justify-between py-6">
<div className="flex justify-between">
<h1 className="text-xl font-bold">{board.title}</h1>
{board.image && (
<div className="rounded-lg border">
<Image
src={board.image}
alt={board.title}
width={72}
height={72}
className="size-[72px]"
/>
</div>
)}
</div>
<div className="flex gap-x-2">
<Image src={heartIcon} alt="heart icon" />
<p>{board.likeCount}</p>
<div className="flex items-center justify-between">
<div className="flex gap-x-2">
<Image src={profileIcon} alt="profile icon" />
<p>{board.writer.nickname}</p>
<p className="text-[--cool-gray400]">
{formatDate(board.createdAt)}
</p>
</div>
<div className="flex gap-x-2">
<Image src={heartIcon} alt="heart icon" />
<p>{board.likeCount}</p>
</div>
</div>
</div>
</div>
<hr />
<hr />
</Link>
Copy link
Collaborator

Choose a reason for hiding this comment

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

@CitrusSoda

이 요소도 하나의 독립적인 콤포넌트로 가져가도 좋겠어요.

boards.tsx라는 콤포넌트에서 board.tsx 콤포넌트를 배열로 반환하도록 하는게 조금 더 가독성이 높아질 거 같아요!

Copy link
Collaborator

Choose a reason for hiding this comment

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

@CitrusSoda

ㅋㅋㅋ 엄청 좋은데요!

Comment on lines +10 to +17
interface DropdownProps {
children: React.ReactNode;
}

interface DropdownItemProps {
children: React.ReactNode;
onClick: () => void;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

@CitrusSoda

이렇게 composition 목적의 콤포넌트를 작성하는 경우 여러 인터페이스와 타입들이 사용되는데요,
이 경우 각 ui 콤포넌트의 구현체 바로 상위에 해당 콤포넌트의 인터페이스를 정의해주는게 가독성에 조금 더 도움이 될 수 있지 않을까 생각되어요

Comment on lines +78 to +79
// ! e의 타입을 MouseEvent, React.MouseEvent 등 다양하게 줘보았으나 해결하지 못하여
// ! unknwon으로 설정하고 instanceof로 MouseEvent일 때 접근가능하도록 하였습니다.
Copy link
Collaborator

Choose a reason for hiding this comment

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

@CitrusSoda

react에서 제공하는 mouseevent말구, 그냥 mouseevent로 타입 설정하면 의도했던대로 동작이안되나요?
https://developer.mozilla.org/ko/docs/Web/API/MouseEvent

Copy link
Collaborator

Choose a reason for hiding this comment

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

@CitrusSoda

UI쪽 작업은 잘하셨어요

다만 로직쪽에서 조금 난잡한게 보이는데요, 우리 add-board.tsx라는 폼에서 어떤 작업을 제공하는지를 한번 정의해보면

  1. 렌더될 폼 UI
  2. 입력된 값에 따른 validation 및 이에 대한 UI 피드백
  3. 제출버튼 클릭 시 제출 로직 실행
  4. 제출 후 결과에 따른 로직 실행

으로 분리를 해볼 수 있겠어요.

1번과 2번은 잘 작성이 되어있습니다.
마찬가지로 3번과 4번도 잘 구현이 되어있는데요, 여기서 관심사 분리 차원에서 조금만 나눠 생각해볼까요?

이 콤포넌트가 관리해야 할 요소를 생각해보면,

  • 폼에 입력된 데이터 관리
  • 이에 따른 밸리데이션 및 UI 피드백
  • onClick / onSubmit시 실행될 함수 정의

가 되겠죠?

그리고, 실제 밸리데이션에 사용될 로직이나, onSubmit시 실행될 함수의 경우, 이 콤포넌트에서 관리가 되기보단 각 도메인으로 나누어 관리되는게 조금 더 유지보수성이 높아질겁니다.

예를 들어, 우리가 지금은 하드코딩으로 validation처리를 하다가 type safety를 위해 zod나 class validator와 같은 라이브러리를 사용한다고 가정해볼게요. 그럼 이 수정사항을 반영하기 위해 이 콤포넌트에 새로운 라이브러리가 import되고, 그 구현체를 위해 우리 코드가 더 뚱뚱해지겠죠?
반면 validation을 하나의 모듈로 분리해 처리를 하고, 우리 콤포넌트에서는 validation을 호출해 그 결과만 받아오도록 하면 validation을 처리하는 모듈에서 class validator를 사용하든 zod를 사용하든 별 상관없이 그냥 우리는 그 모듈만 사용하면 되니까 변경에 대해서 굉장히 유연하게 대처가 되지 않을까 생각이 되어요.

마찬가지로, 폼 제출 시 실행될 로직의 경우, 이 로직을 처리하는 서비스 모듈로 분리를 한다면 제출시 실행될 로직이 얼마나 수정이 되든 상관이 없이 사용할 수 있겠죠.
예를 들어, 지금은 제출 시 네가지 작업을 처리하는데요

  1. 토큰 불러오기
  2. 선택된 이미지가 있다면 이미지를 api통신을 통해 전송
  3. 신규 게시글 생성
  4. 성공 시 페이지 이동 / 실패시 ui 피드백

여기서 만약 한가지라도 수정되면 이 콤포넌트의 로직이 크게 변경이 될거에요.
만약 localStorage가 아니라 쿠키 / 세션에서 토큰값을 불러온다면? 이를위한 라이브러리가 이 콤포넌트에 또 불러와져서 여러 함수들이 구현될거구요
선택된 이미지가 없다면, api콜을 통해 새로운 이미지를 서버에서 인공지능을 활용해 생성하도록 한 후 게시글이 생성되게 하는 기능을 추가해야 한다면, 이 api 콜이 우리 콤포넌트 파일에 또 추가가 되겠죠. 이에대한 에러핸들링도 물론 함께 처리가 되어야 할거구요.

따라서 위와 같은 이유로 로직들을 조금 더 명확하게 나누어 활용해주는게 좋을 것 같습니다!

Comment on lines +27 to +28
const BoardRes = await axios.get(`/articles/${id}`);
const board = BoardRes.data;
Copy link
Collaborator

Choose a reason for hiding this comment

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

@CitrusSoda

특별한 목적이 있는게 아니라면 변수명을 대문자로 시작하지 말아주세요! 만약 반복되는 변수명으로 인해 어쩔수 없이 alias처리를 한거라면 다음과 같은 형태가 조금 더 직관적이리라 생각되는데 한번 체크 부탁드릴게요

const response = await axios.get(..);
const board = response.data;

// 또는
const { data: board } = await axios.get(..)

Comment on lines +131 to +145
<div className="mt-10">
<form onSubmit={handleSubmit(onSubmit)} className="flex flex-col">
<h1 className="font-semibold">댓글 달기</h1>
<textarea
{...register('content')}
placeholder="댓글을 입력해주세요."
className="my-4 h-[104px] resize-none overflow-hidden rounded-xl bg-[--cool-gray100] px-6 py-4"
/>
<input
type="submit"
value="등록"
className={`ml-auto rounded-lg px-6 py-3 text-white ${commentContent ? 'cursor-pointer bg-blue-500' : 'bg-[--cool-gray400]'}`}
disabled={!commentContent}
/>
</form>
Copy link
Collaborator

Choose a reason for hiding this comment

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

@CitrusSoda

댓글작성을 위한 폼 콤포넌트는 다른 콤포넌트로 분리하는게 더 좋아보여요

게시글 상세 정보는 디테일 정보만 관리하도록 하고,
여기에 달리는 댓글달기의 경우 댓글생성 콤포넌트를 따로 만들어 조립하는식으루 하구,
게시글에 달려있는 댓글들도 댓글 콤포넌트를 만들어 붙여주는게 좋겠어요.

Comment on lines +21 to +31
try {
const res = await axios.post('/auth/signIn', data);
if (res.status === 200) {
localStorage.setItem('accessToken', res.data.accessToken);
router.push('/');
} else {
console.log('로그인 실패');
}
} catch (error) {
console.error('로그인 중 오류가 발생하였습니다', error);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

@CitrusSoda

여기서도 마찬가지로 로직 분리를 해보면 어떨까요?

@1005hoon 1005hoon merged commit dd3d36a 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