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

Conversation

eunbinnie
Copy link
Collaborator

요구사항

기본

[sprint10]

  • 게시글 등록 페이지 주소는 “/addboard” 입니다.
  • 게시판 이미지는 최대 한개 업로드가 가능합니다.
  • 각 input의 placeholder 값을 정확히 입력해주세요.
  • 이미지를 제외하고 input 에 모든 값을 입력하면 ‘등록' 버튼이 활성화 됩니다.
  • 게시글 상세 페이지 주소는 “/board/{id}” 입니다.
  • 댓글 input 값을 입력하면 ‘등록' 버튼이 활성화 됩니다.
  • 자유게시판 페이지에서 게시글을 누르면 게시물 상세 페이지로 이동합니다.

멘토에게

  • 요구사항만 만족한 상태인데 이어서 작업하겠습니다..ㅎ

@eunbinnie eunbinnie requested a review from kiJu2 June 21, 2024 13:55
@eunbinnie eunbinnie added the 매운맛🔥 뒤는 없습니다. 그냥 필터 없이 말해주세요. 책임은 제가 집니다. label Jun 21, 2024
@kiJu2
Copy link
Collaborator

kiJu2 commented Jun 25, 2024

수고 하셨습니다 ! 스프리트 미션 하시느라 정말 수고 많으셨어요.
학습에 도움 되실 수 있게 꼼꼼히 리뷰 하도록 해보겠습니다.

Comment on lines -3 to -9
export interface GetArticlesQuery {
page?: number;
pageSize?: number;
orderBy: "recent" | "like";
keyword?: string;
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

굳굳 !! 다른 곳으로 빼셨군요 👍


useEffect(() => {
const { title, content } = articleValues;
title !== "" && content !== "" ? setDisabled(false) : setDisabled(true);
Copy link
Collaborator

Choose a reason for hiding this comment

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

삼항 연산자는 기본적으로 가독성이 좋지 못합니다 !

다음과 같이 코드를 작성할 수 있어요:

Suggested change
title !== "" && content !== "" ? setDisabled(false) : setDisabled(true);
if (title !== "" && content !== "") {
setDisabled(false);
} else {
setDisabled(true);
}

Copy link
Collaborator

Choose a reason for hiding this comment

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

그리고 if 문 안에서 분기를 나누고 true/false를 직접 기입하는 경우:

setDisabled(title === "" || content === "");

위와 같이 조건문 자체를 변수로 활용할 수 있습니다 ! 😊

};

useEffect(() => {
comment !== "" ? setDisabled(false) : setDisabled(true);
Copy link
Collaborator

Choose a reason for hiding this comment

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

여기도 삼항연산자군요 !

한 번 위에 예시대로 직접 조건문을 변수로 활용하여 바꿔보시겠어요?

Comment on lines +36 to +41
export interface GetArticlesQuery {
page?: number;
pageSize?: number;
orderBy: "recent" | "like";
keyword?: string;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

음 해당 타입은 API 함수와 가까이 있는게 어떨까요?

API에 필요한 파라메터 타입을 지정한 것 같아요. API 쿼리에 필요한 파라메터가 맞다면 API 함수와 가까이 둬보시고 사용해볼까요 ?

href,
activePaths,
children,
}: PropsWithChildren<LinkType>) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

오올 굳굳 !! 리액트 내장 타입을 사용하셨군요.

이미 정의되어 있다면 굳이 다시 정의할 필요 없지요 ㅎㅎㅎ

className="absolute top-2/4 left-5 -translate-y-2/4"
/>
<input
type="search"
Copy link
Collaborator

Choose a reason for hiding this comment

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

굳굳 ! search 타입을 사용하셨군요 👍

<form onSubmit={handleSubmitSearch} className="flex-1 relative">
<Image
src={searchIcon}
alt="검색"
Copy link
Collaborator

Choose a reason for hiding this comment

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

단순 장식용으로 있을 경우 ""로 나타낼 수 있습니다 !

Suggested change
alt="검색"
alt=""

장식 이미지는 페이지 콘텐츠에 정보를 추가하지 않습니다. 예를 들어, 이미지에서 제공하는 정보는 인접한 텍스트를 사용하여 이미 제공될 수도 있고, 웹 사이트를 시각적으로 더욱 매력적으로 만들기 위해 이미지가 포함될 수도 있습니다.

이러한 경우 스크린 리더와 같은 보조 기술에서 무시할 수 있도록 null(빈) alt텍스트를 제공해야 합니다( ). alt=""이러한 유형의 이미지에 대한 텍스트 값은 화면 판독기 출력에 청각적 혼란을 추가하거나 주제가 인접한 텍스트의 주제와 다른 경우 사용자의 주의를 산만하게 할 수 있습니다. 속성 을 생략하는 alt것도 옵션이 아닙니다. 속성이 제공되지 않으면 일부 화면 판독기가 이미지의 파일 이름을 대신 알려주기 때문입니다.

Decorative Images

Comment on lines +8 to +11
const SubmitButton = ({
disabled,
children,
}: PropsWithChildren<ISubmitButtonProps>) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

버튼 타입은 이미 정의되어 있어요 !:

Suggested change
const SubmitButton = ({
disabled,
children,
}: PropsWithChildren<ISubmitButtonProps>) => {
const SubmitButton = ({
disabled,
children,
}: React.ButtonHTMLAttributes<HTMLButtonElement>) => {

위와 같이 정의해볼 수 있습니다. 😊 disabeldchildren은 이미 해당 타입에 존재합니다 !

@kiJu2
Copy link
Collaborator

kiJu2 commented Jun 25, 2024

훌륭합니다 은빈님 ! 이 전의 피드백 정말 잘 반영해주셨군요 😊👍
수고 정말 많으셨어요. 리뷰 중 궁금하신거 있으시면 사전 질의를 통해서 남겨주시거나 멘토링 미팅 때 질문주세요 ㅎㅎㅎ

@kiJu2 kiJu2 merged commit 4c6c1d2 into codeit-bootcamp-frontend:Next.js-이은빈 Jun 25, 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