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

[김은재] Week13 #424

Conversation

rladmswo1715
Copy link
Collaborator

@rladmswo1715 rladmswo1715 commented May 13, 2024

요구사항

기본

  • React 프로젝트에서 진행했던 작업물을 Next.js 프로젝트에 맞게 변경 및 이전해 주세요.

심화

  • [x]
  • []

주요 변경사항

스크린샷

멘토에게

  • folder 페이지에서 공유 버튼을 클릭 할 때 key값을 줘도 자꾸 key prop경고가 뜨는데 이유를 모르겠습니다..

image

@rladmswo1715 rladmswo1715 requested a review from choinashil May 13, 2024 05:02
@rladmswo1715 rladmswo1715 added 매운맛🔥 뒤는 없습니다. 그냥 필터 없이 말해주세요. 책임은 제가 집니다. and removed 매운맛🔥 뒤는 없습니다. 그냥 필터 없이 말해주세요. 책임은 제가 집니다. labels May 13, 2024
Copy link
Collaborator

@choinashil choinashil left a comment

Choose a reason for hiding this comment

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

은재님 next.js 랑 타입스크립트 아직 익숙하지 않으실 수 있는데 전반적으로 깔끔하게 잘 구현해주셨네요!
중복되는 타입이나 로직 분리도 잘 되어있고 컴포넌트도 재사용성 고려해서 설계하신 것이 느껴집니다
이후 미션과 프로젝트도 화이팅입니다! 👍🏽

@@ -0,0 +1,18 @@
import { BASE_URL } from "@/constants/url";

export const getSignInProfile = async () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

파일명을 header 보다는 user 나 auth (authentication) 으로 하는게 더 자연스러울 것 같아요

헤더에서 사용되는 거라서 이렇게 지으셨을 것 같은데
헤더가 아닌 다른 곳에서 사용되게 되면 맞지 않게 되니까 사용처보다는 api 의 자원이나 용도를 기준으로 파일 구분 및 파일 네이밍을 하면 좋을 것 같습니다 :)

Comment on lines +16 to +19
${({ usetype }) =>
(usetype === "folderAdd_modal" ||
usetype === "folderNameChange_modal" ||
usetype === "linkAdd_modal") &&
Copy link
Collaborator

Choose a reason for hiding this comment

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

버튼을 용도 별로 타입 구분을 하셨는데요,
프로젝트가 커지면 버튼의 사용처가 훨씬 많아질텐데 그 때마다 계속 그에 맞는 타입을 추가하면 번거로울 것 같아요.

보통은 primary, secondary, outlined.. 이런 식으로 용도 보다는 스타일 자체로 구분을 하고
사용처마다 원하는 스타일을 골라서 쓰는 식으로 많이 작업합니다

보통 이 경우에는 컴포넌트 디자인부터 위와 같이 정리가 되어있어서 개발에서도 동일하게 구현을 하는데요,
미션에서는 그렇게까지 정리가 안되어있어서 스타일 별로 정리하기 어려운 부분도 있을 것 같긴 하네요
참고만 해주시면 됩니다!

color: #6D6AFE;
background-color: #E7EFFB;
}
`
Copy link
Collaborator

Choose a reason for hiding this comment

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

파일의 제일 하단에는 빈 줄을 하나 넣어주세요
End of Line, EOL 과 같은 키워드로 찾아보시면 됩니당

const [isKebabOpen, setIsKebabOpen] = useState(false);

const handleKebabClick = (e: React.MouseEvent) => {
e.preventDefault();
Copy link
Collaborator

Choose a reason for hiding this comment

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

여기서 preventDefault 를 꼭 써야하는 이유가 있나요??
의미없이 추가됐거나 없어도 동일하게 동작하는 코드라면 삭제하는 것이 좋습니다!
물론 의도가 있었다면 유지해도 됩니당

e.preventDefault();
e.stopPropagation();

isKebabOpen ? setIsKebabOpen(false) : setIsKebabOpen(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
isKebabOpen ? setIsKebabOpen(false) : setIsKebabOpen(true);
setIsKebabOpen(!isKebabOpen);

navName="전체"
onClick={onClickNavItem}
navId={9999}
isCurrentNav={navId === 9999 ? true : false}
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
isCurrentNav={navId === 9999 ? true : false}
isCurrentNav={navId === 9999}

Comment on lines +11 to +25
const snsInfo = {
url: [
"https://www.facebook.com/",
"https://twitter.com/",
"https://www.youtube.com/",
"https://www.instagram.com/",
],
img: [icon_facebook, icon_twitter, icon_youtube, icon_instagram],
imgAlt: [
"페이스북 아이콘",
"트위터 아이콘",
"유튜브 아이콘",
"인스타그램 아이콘",
],
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

여기도 위에 보여드린 구조로 변경하면 좋을 것 같아요

(param: ModalParam | null) => {}
);

function Modal({ children }: 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 로 하셨나요 ㅋㅋ

Comment on lines +14 to +15
<ModalSetterContext.Provider value={setState}>
<ModalStateContext.Provider value={state}>
Copy link
Collaborator

Choose a reason for hiding this comment

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

두개 나누셨군요 👍🏽

@@ -0,0 +1,14 @@
import { useEffect } from "react";

const useModalScrollBlock = () => {
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 +88 to +102
<S.NavSettingBox>
<span>{currentNav}</span>
{navId !== 9999 && (
<div>
<button
onClick={() =>
handleOpenModal({
type: "share",
props: { title: "폴더 공유", subTitle: currentNav },
})
}
>
<Image src={shareIcon} alt="공유 아이콘" />
<span>공유</span>
</button>
Copy link
Collaborator

Choose a reason for hiding this comment

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

folder 페이지에서 공유 버튼을 클릭 할 때 key값을 줘도 자꾸 key prop경고가 뜨는데 이유를 모르겠습니다..

코드 상 '공유' 버튼은 이 부분인 것 같은데 여기는 map 도는 부분이 없어서 key prop 과 관련이 없어보이는데용??
이 부분과 관련된 에러가 맞나요??

혹시 다른 map 도는 부분에서 key 를 넘겨줘도 에러가 난다면
동일한 값이 여러 아이템의 key 로 전달되고 있어서 그런걸 수도 있으니 확인해보세요
(ex. 중복된 id 가 있는데 id 를 key 로 쓰고 있다거나)

참고로 다음에는 에러 공유해주실 때 에러 메세지도 같이 복붙 또는 캡쳐해서 보여주시면 파악하기 더 좋을 것 같습니다!

@choinashil choinashil merged commit 457dcd1 into codeit-bootcamp-frontend:part3-김은재 May 14, 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