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 #663

Open
wants to merge 28 commits into
base: main
Choose a base branch
from

Conversation

gohansaem1
Copy link
Collaborator

요구사항

기본

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

심화

  • [x]
  • []

주요 변경사항

스크린샷

image

멘토에게

  • addboard/{id} 개별 게시글 접근할 때 useRouter 쓰는데 헤맸고, 조건문을 걸어서 해결했습니다.
  • 아직 미완성이라, 계속 기능 구현 할게요.
  • 셀프 코드 리뷰를 통해 질문 이어가겠습니다.

@gohansaem1 gohansaem1 requested a review from kich555 June 7, 2024 15:02
@gohansaem1 gohansaem1 added the 미완성🫠 죄송합니다.. label Jun 7, 2024
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.

고생하셨습니다!!
전반적으로 괜찮게 잘 해주셨어요! ㅎㅎ
다만 타입스크립트를 사용하실때 타입을 선언해주시는 부분에 주의해주세요!
👍👍👍

height: 42px;
border-radius: 8px;
background-color: #3692ff;
font-family: Pretendard;
Copy link
Collaborator

Choose a reason for hiding this comment

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

해당 버튼에만 Pretendard가 적용되어야 하나요?
그런게 아니라면 font는 최상위 node에서 사용하는게 좋을것 같습니다 ㅎ

@@ -0,0 +1,5 @@
import styles from "@/components/BlueButton.module.css";

export default function BlueButton({ children }) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

export const BlueButton:React.FC<PropsWithChildren> = ({ children }) => {

이렇게하면 타입을 지정할 수 있어요

@@ -22,6 +22,7 @@ const SearchForm: React.FC<SearchFormProps> = ({

if (!value) {
router.push("/boards");
onChangeKeyword(" ");
Copy link
Collaborator

Choose a reason for hiding this comment

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

empty string이 아니라 공백으로 처리하는게 맞나요?

@@ -1,5 +1,9 @@
import styles from "@/components/BlueButton.module.css";

export default function BlueButton({ children }) {
return <div className={styles.write}>{children}</div>;
export default function BlueButton({ type = "", children }) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

타입스크립트를 사용한다면 타입을 꼭 지정해주세요!

<CommentItem comments={comments} />
</>
);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

export function CommentList({ comments = [] }) {
  return (...

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

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

그리고 따로 만든 컴포넌트들은 왠만하면 다른 파일로 구분해주시는게 좋을것 같네요 최대한 pages내부의 파일에는 해당 파일에 대한
export default만 존재하는게 더 좋을것 같아요 ㅎ 그 이유는 해당 파일이 단순히 파일의 역할만 하는게 아니라 next js 내부의 동적 라우팅을 담당하기 때문이에요 ㅎ

}
async function getComments(articleId) {
if (!articleId) return;
const res = await axios.get(`/articles/${articleId}/comments?limit=5`);
Copy link
Collaborator

Choose a reason for hiding this comment

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

const res = await axios.get(`/articles/${articleId}/comments`, params: {
  limit: 5
});

axios를 사용하신다면 위와같이 searchParam을 객체로 뺄 수 있어요 ㅎ

async function getComments(articleId) {
if (!articleId) return;
const res = await axios.get(`/articles/${articleId}/comments?limit=5`);
const nextComments = 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.

 const nextComments = res.data.list;

과 같이 axios결과값을 한번더 변수에 할당하는건 불필요해 보입니다 ㅎ

그리고 axios가 반환하는 data에는 비동기 함수의 결과값이 담기는데요,
서버 통신하는 비동기함수는 언제나 실패할 수 있다는 점을 염두해두어야합니다.
api가 만약 에러를 throw하면 res.data는 undefined가 되고 그러면 undefined.list 가 되어 런타임 에러가 발생할거에요
언제나 axios가 반환하는 data는 undefiend가 될 수 있다는 점을 꼭 주의해주세요!

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.

4 participants