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

[김민섭] Week8 #342

Conversation

striker1826
Copy link
Collaborator

요구사항

기본

  • PC, Tablet, Mobile 디자인에 해당하는 폴더 페이지를 만들어 주세요.
  • 폴더 페이지에서 저장된 링크가 없는 경우의 디자인도 만들어 주세요.
  • 링크 공유 페이지 url path는 ‘/shared’, 폴더 페이지 url path는 ‘/folder’가 되도록 설정해 주세요.
  • 링크 추가하기, 폴더 추가, 공유, 이름 변경, 삭제 버튼들의 기능은 추후 만듭니다.
  • React를 사용해 진행합니다.

멘토에게

@striker1826 striker1826 requested a review from clianor April 8, 2024 02:58
@striker1826 striker1826 added the 순한맛🐑 마음이 많이 여립니다.. label Apr 8, 2024
Comment on lines +31 to +33
border: 1px solid var(--Linkbrary-primary-color, #6d6afe);
background: var(--Linkbrary-white, #fff);
color: var(--Linkbrary-gray60, #9fa6b2);
Copy link
Collaborator

Choose a reason for hiding this comment

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

css 변수로 작성된 부분도 테마를 이용한 방법으로 개선해 보시면 어떨까요?

};

const CategoryBtn = ({ folderId, currentFolderId, setFolderId, setSelectedCategory, children }) => {
const [isSeleted, setIsSelected] = useState(false);
Copy link
Collaborator

Choose a reason for hiding this comment

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

버튼의 선택된 여부를 상위 컴포넌트에서 관리하고, 해당 상태를 자식 컴포넌트에 props로 전달하는 것이 좋습니다.
자식 컴포넌트 내에서 선택 상태를 관리하면, 여러 버튼이 동시에 선택될 수 있는 문제가 발생할 수 있기 때문입니다.
상태 관리는 상위 컴포넌트에서 집중적으로 이루어지고, 자식 컴포넌트에는 해당 상태를 전달하는 것이 좋습니다.

const [folderData] = useGetFolder();
const [folderList] = useGetFolderList();
const [linkList] = useGetLinks(1, folderId);
const [selectedCategory, setSelectedCategory] = useState(folderList[0].name);
Copy link
Collaborator

Choose a reason for hiding this comment

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

해당 상태가 관리되어야할 이유가 보이지 않습니다.
아래와 같이 상태가 관리 되는 folderList와 folderId를 이용해 값만 추출하시는게 좋은 방향으로 보여집니다.
const selectedCategory = folderList.find((folder) => folder.id === folderId).name ?? folderList[0].name

const CategoryBtn = ({ folderId, currentFolderId, setFolderId, setSelectedCategory, children }) => {
const [isSeleted, setIsSelected] = useState(false);

useChangeFolderColor(currentFolderId, folderId, setIsSelected);
Copy link
Collaborator

Choose a reason for hiding this comment

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

해당 커스텀 훅은 상태를 부모 요소로 끌어올리게 되면 더이상 필요가 없어질 것으로 보여집니다.

Copy link
Collaborator

Choose a reason for hiding this comment

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

해당 커스텀 훅은 유틸리티 함수로 작성하는 것이 더 적절할 것 같습니다.
훅은 리액트의 생명주기와 밀접하게 연관되어 있으며, 상태 관리와 같은 특정 기능을 수행하기 위해 나왔습니다.
반면, 유틸리티 함수는 범용적인 로직을 캡슐화하기 위해 사용되므로, 해당 기능이 리액트 컴포넌트의 생명주기와 직접적인 연관이 없다면 유틸리티 함수로 작성하는 것이 더 바람직한 방향일것 같습니다.

Copy link
Collaborator

Choose a reason for hiding this comment

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

useFormattingDate.jsx 파일의 리뷰를 참고해주세요.

@clianor
Copy link
Collaborator

clianor commented Apr 10, 2024

이번에도 고생하셨습니다!

@clianor clianor merged commit ea19988 into codeit-bootcamp-frontend:part2-김민섭 Apr 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.

4 participants