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

Merged
merged 10 commits into from
Apr 10, 2024
Merged

[황은지] week8 #318

merged 10 commits into from
Apr 10, 2024

Conversation

eunji-0623
Copy link
Collaborator

요구사항

기본

  • 링크 공유 페이지 url path는 ‘/shared’, 폴더 페이지 url path는 ‘/folder’가 되도록 설정했나요?
  • 폴더 페이지에서 상단 네비게이션 바는 스크롤 시 상단에 고정하지 않고 가려지도록 했나요?
  • 상단 네비게이션 바에 프로필 영역의 데이터는 https://bootcamp-api.codeit.kr/docs 에 명세된 “/api/users/1”을 활용했나요?
  • “전체” 폴더를 선택한 경우 “공유”, “이름 변경”, “삭제” 버튼들이 보이지 않지만, 다른 폴더를 선택한 경우에는 버튼들이 보이나요?
  • 폴더 목록에 필요한 데이터는 “/api/users/1/folders”를 활용했나요?
  • “전체” 폴더에 필요한 링크들 데이터는 “/api/users/1/links”를 활용하고, 이외의 폴더에 필요한 링크들 데이터는 “/api/users/1/links?folderId={해당 폴더 ID}”를 활용했나요?
  • Mobile에서 “폴더 추가” 버튼은 최하단에서 101px 떨어져있는 Floating Action Button으로 만들었나요?

심화

주요 변경사항

스크린샷

image

멘토에게

@eunji-0623 eunji-0623 changed the base branch from main to part2-황은지 April 5, 2024 09:32
@eunji-0623 eunji-0623 requested a review from clianor April 5, 2024 09:32
@eunji-0623 eunji-0623 added the 순한맛🐑 마음이 많이 여립니다.. label Apr 5, 2024
Copy link
Collaborator

Choose a reason for hiding this comment

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

잘해주셨습니다.
그러나 이제 api를 처리하는 함수가 많아진 만큼 요청 성공, 실패 체크 로직이 반복되고 있습니다.
이런 부분을 별도의 fetcher 함수로 분리하여 공통 로직을 하나의 함수에서 처리하도록 수정해보는건 어떨까요?
이렇게 되면 코드의 중복이 제거되고, 각각의 api 요청하는 함수의 가독성이 향상되며, 재사용성이 올라가고, 에러 처리를 공통된 fetcher에서 처리할 수 있어 에러 처리의 일관성이 향상될 수 있습니다.

Copy link
Collaborator

Choose a reason for hiding this comment

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

커스텀 훅을 별도의 파일로 분리해서 호출하도록 하는건 어떨까요?
App.js 파일이 내에 너무 많은 코드가 있는것 같습니다.

@@ -7,13 +7,14 @@ const SIZES = {
};

const GradientButton = styled.button`
width: ${({ size }) => SIZES[size] ?? SIZES["medium"]}px;
padding: 16px 20px;
width: ${({ size }) => SIZES[size ?? "medium"]}px;
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
width: ${({ size }) => SIZES[size ?? "medium"]}px;
width: ${({ size = "medium" }) => SIZES[size]}px;

Comment on lines +102 to +103
{isFolderPage ? <Icon width="34" height="34" top="15" img={starIcon} /> : null}
{isFolderPage ? <Icon width="21" height="17" top="215" img={kebabIcon} /> : null}
Copy link
Collaborator

Choose a reason for hiding this comment

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

삼항 연산자를 이용해서 작성해주신 부분이 좋았습니다.
앞으로도 JSX 문법 내에서 삼항 연산자를 이용해주세요.

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.

컴포넌트 파일의 명칭과 export 되는 함수의 명칭을 통일하는 것이 일반적으로 권장 되는 관행으로 알고 있습니다.
파일 이름과 export 되는 컴포넌트 명칭이 달라 해당 파일 내에서 코드의 시작점이 눈에 들어오지 않습니다.

setLinkList({ data });
})();
}, [folderId]);
if (!linkList.data) return;
Copy link
Collaborator

Choose a reason for hiding this comment

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

잘못된 방법은 아니나 리액트에서 컴포넌트 내에서 early return을 할때 return null;을 사용하는 것이 더 선호되는 방식으로 알고 있습니다.
지금도 잘못된건 아닙니다!

@clianor
Copy link
Collaborator

clianor commented Apr 10, 2024

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

@clianor clianor merged commit acefd76 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.

2 participants