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

[허찬욱] week11 #370

Merged
merged 2 commits into from
May 2, 2024
Merged

[허찬욱] week11 #370

merged 2 commits into from
May 2, 2024

Conversation

hkwangro
Copy link

요구사항

기본

  • [0]케밥 버튼을 누르면 팝오버가 보이게 해주세요.

심화

  • [x]
  • []

주요 변경사항

스크린샷

image

멘토에게

  • 셀프 코드 리뷰를 통해 질문 이어가겠습니다.

@hkwangro hkwangro requested a review from kiJu2 April 30, 2024 12:23
@hkwangro hkwangro added the 미완성🫠 죄송합니다.. label Apr 30, 2024
@hkwangro hkwangro changed the title feat: 기능 구현 [허찬욱]week11 Apr 30, 2024
@hkwangro hkwangro changed the title [허찬욱]week11 [허찬욱] week11 Apr 30, 2024
@kiJu2
Copy link
Collaborator

kiJu2 commented May 1, 2024

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

import { useAsync } from "sharing/util";
import { axiosInstance } from "sharing/util";

export const useGetFolder = () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

굳굳 ! API마다 커스텀 훅을 만드셨군요 ! 👍

@@ -0,0 +1,14 @@
import { mapLinksData } from "link/util-map";

export const mapFolderData = (folder) => {
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 useGetFolder = () => {
const getFolder = () => axiosInstance.get("sample/folder");
const { loading, error, data } = useAsync(getFolder);
Copy link
Collaborator

Choose a reason for hiding this comment

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

loading, error, data 등 비동기에 필요한 상태관리가 컴팩트하게 잘 이루어져있군요 ! 👍

const { loading, error, data } = useAsync(getFolders);

const folders = data?.data ?? [];
const sortedFolders = folders.sort((a, b) => a?.id - b?.id);
Copy link
Collaborator

Choose a reason for hiding this comment

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

정렬은 함수로 따로 구분할 수 있을 것 같아요.

useGetFolders의 책임이 두 가지 이상이 되는 것 같아서요 ! 물론, 모두 다 코드를 분리하게되면 생산성이 좋지 못하기에 참고 정도로만 봐주시면 될 것 같습니다 😊


const cx = classNames.bind(styles);

export const AddFolderButton = ({ onClick }) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

해당 컴포넌트는 IconButton과 같이 확장성 있는 컴포넌트로 만들어볼 수 있을 것 같아요:

Suggested change
export const AddFolderButton = ({ onClick }) => {
export const IconButton = ({ onClick, icon }) => {

import { CardContent } from "sharing/ui-card-content";
import { CardImage } from "sharing/ui-card-image";

export const ReadOnlyCard = ({ url, imageSource, alt, elapsedTime, description, createdAt }) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

훌륭합니다 !

ui 계층을 분리하여 Card를 사용하고 합성하여 새로운 ReadOnlyCard를 만드셨군요 ! 👍👍

return (
<div className={cx("container")}>
<input className={cx("input")} type="search" placeholder="링크를 검색해 보세요." />
<img src={SEARCH_IMAGE} alt="검색창인 것을 알려주는 돋보기 아이콘" className={cx("icon")} />
Copy link
Collaborator

Choose a reason for hiding this comment

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

alt가 자세하고 좋아요 🥺🥺🥺

const { data: folders } = useGetFolders();
const [selectedFolderId, setSelectedFolderId] = useState(ALL_LINKS_ID);
const { data: links, loading } = useGetLinks(selectedFolderId);
const cardList = useMemo(() => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

useMemo를 활용한 최적화 👍👍👍

return (
<CardList>
{links.map((link) => (
<EditableCard key={link?.id} {...link} />
Copy link
Collaborator

Choose a reason for hiding this comment

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

key 값이 매우 적절합니다 !

<span className={cx("copyright")}>{TEXT.codeit}</span>
<div className={cx("links")}>
<a className={cx("link")} href={ROUTE.개인정보처리방침}>
{TEXT.privacyPolicy}
Copy link
Collaborator

Choose a reason for hiding this comment

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

혹시 TEXT라는 상수를 따로 정의한 이유가 있으실까요?

상수로 파일을 분리하면서 이득을 얻을 수 있는게 무엇이 있을지 궁금합니다 !

@kiJu2
Copy link
Collaborator

kiJu2 commented May 1, 2024

수고 많으셨습니다 찬욱님 ! ㅎㅎㅎ
컨플릭트 해결해주시면 바로 머지하겠습니다 ~!

@hkwangro hkwangro closed this May 1, 2024
@hkwangro hkwangro reopened this May 1, 2024
@kiJu2 kiJu2 merged commit cf3295d into codeit-bootcamp-frontend:part2-허찬욱 May 2, 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