-
Notifications
You must be signed in to change notification settings - Fork 24
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 #135
base: next-함헌규
Are you sure you want to change the base?
The head ref may contain hidden characters: "next-\uD568\uD5CC\uADDC-sprint10"
[함헌규] sprint10 #135
Conversation
- 로그인,회원가입 페이지를 (auth) 폴더에 배치하고 layout 파일 추가
- page의 초기값으로 1이 아닌 initialPage 를 입력: searchParams의 변경으로 리렌더링이 일어날 때 초기값인 1로 초기화되어 페이지가 1로 변경되는 문제점 발견 searchParams의 skip값을 가져와서 skip값에 따라 초기 페이지값을 결정
- localStorage 사용을 위한 변경
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.
안녕하세요. 헌규님.
제가 프론트 PR에도 리뷰를 남기는 걸 깜빡했네요. 늦어져서 죄송합니다.
프론트엔드 쪽은 저번에 남겼던 리뷰에서 크게 달라지는 부분은 없을 것 같네요.
몇 가지 추가로 보이는 부분들 남겨두었습니다. (저번에 달았던 것 기억 못하고 다시 남긴 내용이 있을 수도 있습니다.)
그리고 이제 백엔드와 프론트엔드를 동시에 작업을 들어가니 백엔드에서 만들어둔 인터페이스를 프론트엔드에서도 활용해보면 어떨까요?
프론트와 백엔드가 둘 다 TS 기반일 때 누릴 수 있는 가장 큰 장점이죠. ㅋㅋ
return ( | ||
<> | ||
<ArticleHeader | ||
id={article.id} | ||
// 이후 닉네임 입력 예정 | ||
nickname='판다' | ||
title={article.title} | ||
createdAt={article.createdAt} | ||
/> | ||
<div className='mb-8'>{article.content}</div> | ||
</> | ||
article && ( | ||
<> | ||
<ArticleHeader | ||
ownerId={article.writer.id} | ||
id={article.id.toString()} | ||
nickname={article.writer.nickname} | ||
title={article.title} | ||
createdAt={article.createdAt} | ||
likeCount={article.likeCount} | ||
isLiked={article.isLiked} | ||
/> | ||
<div className='mb-8'>{article.content}</div> | ||
</> | ||
) | ||
); |
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.
article이 존재하는 상황에만 출력되는 컴포넌트라면 반대로 article 데이터가 없을 때 먼저 null을 반환시켜버리면 이 구분이 조금 더 깔끔해질 것 같습니다.
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.
저는 이왕이면 하나의 컴포넌트 내에서는 동일한 추상화 수준을 맞춰주는 걸 선호하는데요.
현재 ArticleHeader는 다양한 요소를 포함하고 있어서 확실하게 컴포넌트로 만들어 두셨는데, content 영역의 경우는 클래스를 할당한 div 하나라 그대로 사용하고 있는 것으로 추측됩니다. 이런 상황에서 하나인 div라도 ArticleContent로 정의해두면 현재 컴포넌트 내에서는 ArticleHeader와 ArcileContent로 추상화 수준이 동일해지기 때문에 훨씬 더 직관적으로 읽기 쉬워지는 경향이 있습니다.
그리고 이렇게 컴포넌트를 만들어놓고 보면 Content에 비해 Header가 너무 큰 건 아닌가? props 전달 방식이 비효율적이진 않는가? 하는 질문으로 이어지면서 다른 아이디어가 떠오를 수도 있답니다.
<div className='mb-10'> | ||
{isLoading ? ( | ||
<CommentSkeletonList /> | ||
) : comments && comments.list.length > 0 ? ( |
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.
조건에 따라 컴포넌트 3개가 반환되는 것도 그렇지만, 특히 Comment 렌더를 판단하는 조건 로직이 복잡하게 보이네요.
이런 경우라면 render 함수를 하나 정의해서 조건문으로 짧게 쳐내는 것도 좋아보입니다.
현재 판단 구문을 그대로 가져간다면 isLoading이 true일 때 CommentSkeletonList 반환, comments가 없을 때 NoComment를 반환시켜버리면 나머지는 comment가 있다는 조건을 전제로 반환을 시켜버릴 수 있으니 모양이 조금 더 보기 좋아질 것 같네요.
요구사항
기본 요구사항
공통
프론트엔드 구현 요구사항
중고마켓 페이지
상품 등록하기 페이지
심화 요구사항
상태코드 (웹 API 관련)
인증
(생략 가능) 자유게시판 게시물 등록
멘토에게