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

[우지석] Week7 #290

Merged
merged 8 commits into from
Apr 3, 2024
Merged

[우지석] Week7 #290

merged 8 commits into from
Apr 3, 2024

Conversation

jisurk
Copy link
Collaborator

@jisurk jisurk commented Mar 31, 2024

요구사항

기본

  • [기본] 상단 네비게이션 바, 푸터가 랜딩 페이지와 동일한 스타일과 규칙으로 만들어졌나요? (week 1 ~ 3 요구사항 참고)

  • - [ ] [기본] 상단 네비게이션 바에 프로필 영역의 데이터는 https://bootcamp-api.codeit.kr/docs 에 명세된 “/api/sample/user”를 활용했나요?

  • [기본] 상단 네비게이션 바에 프로필 영역의 데이터가 없는 경우 “로그인” 버튼이 보이도록 만들었나요?

  • [기본] 폴더 소유자, 폴더 이름 영역, 링크들에 대한 데이터는 “/api/sample/folder”를 활용했나요?

  • [기본] Static, no image, Hover 상태 디자인을 보여주는 카드 컴포넌트를 만들었나요?

  • [기본] Hover 상태에서 이미지가 기본 크기의 130%로 커지나요?

  • [기본] 카드 컴포넌트를 클릭하면 해당 링크로 새로운 창을 띄워서 이동하나요?

  • [기본] Tablet에서 카드 리스트가 화면의 너비 1124px를 기준으로 이상일 때는 3열로 작을 때는 2열로 배치되나요?

  • [기본] Tablet, Mobile에서 좌우 최소 여백은 32px 인가요?

심화

  • 커스텀 hook을 만들어 활용했나요?

주요 변경사항

스크린샷

image

멘토에게

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

@jisurk jisurk requested a review from 1005hoon March 31, 2024 09:24
@jisurk jisurk self-assigned this Mar 31, 2024
Copy link
Collaborator

@1005hoon 1005hoon left a comment

Choose a reason for hiding this comment

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

@jisurk

지석님! 코드 확인했습니다.

우선 최대한 콤포넌트 화 및 멘토링때 이야기 했던 내용들을 반영하고자 하는게 보이는 코드에요 ㅎㅎ
다만 아직 많이 경험해보지 않으셨다보니 조금 어색한 부분들이 존재합니다.

몇가지 포인트 해보자면

  1. 콤포넌트 네이밍
    스타일 콤포넌트를 활용하는 경우, 두가지 방법으로 함수 선언을 하고는 하는데요
    첫 방법으로는
/components/button/index.jsx
/components/button/styles.jsx

로 만들어

// index.jsx
import {StyledButtonContainer, StyledButtonIcon, StyledButtonText } from './styles.jsx'

export default function Button() {
  return <StyledButtonContainer>
    { icon && <StyledButtonIcon /> }
    <StyledButtonText>{props.children}</ StyledButtonText>
  </StyledButtonContainer>
}

이런 형태로 활용하거나, 지석님이 사용하신것처럼 콤포넌트 내에 바로 선언을 하는 경우가 있습니다. 이는 취향을 따르기 때문에 좀 더 직관적이라 느껴지는 형태를 따르면 되겠고, 다만 컨벤션에 따라 스타일 콤포넌트의 경우 앞에 styled를 붙여주는게 좋을 것 같아요.

  1. use-fetch 훅의 목적과 범위
    이 함수의 경우 데이터 상태를 관리하고자 하면 책임을 벗어나게 됩니다!
    왜냐하면 멘토링떄 이야기했던것처럼 네트워크 통신에서 발생할 수 있는 에러케이스는 너무나도 다양하기 때문에 이런 요소들을 하나의 훅으로 만들어 관리하자는 목적이 있었던거잖아요.
    따라서 이 훅은 네트워크 통신 관련된 처리만 해결하도록 하고, 이에대한 결과물은 UI controller로써동작하는 useEffect 단에서 처리하도록 하는게 좋겠습니다.

  2. Presenter / Container Component
    멘토링때 이야기했던 내용이지만 한번 반복해서 적용해보시면 좋겠어요!
    만약 너무 오래되어 기억나지 않는다면 이 포스트가 완전 단순하지만 그래도 참고해볼만해서 공유드립니다.
    https://ajdkfl6445.gitbook.io/study/react/container-and-presentation

고생 많으셨어요!

Copy link
Collaborator

Choose a reason for hiding this comment

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

@jisurk

폴더명은 특별한 목적이 존재하지 않는 한 소문자와 _ 의 조합으로만 생성해주세요

Copy link
Collaborator

Choose a reason for hiding this comment

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

@jisurk

콤포넌트의 구조를 조금 더 나누어도 좋겠습니다.
보통 카드라는 콤포넌트가 있다면 말 그대로 카드형태의 UI 콤포넌트를 뜻하거든요.

그런데 우리가 작성한 카드 콤포넌트는 카드 UI를 지닌 링크 카드 라는 녀석들을 그리드 형태로 렌더링 해주고 있죠.
조금 더 분리가 가능할 것 같아요.

분리한다면 다음과 같은 형태가 되겠어요.
만약 다른 콤포넌트에서도 카드 UI가 동일하게 사용된다면 components/common/card.jsx 에서 카드 콤포넌트를 만들고
components/link-card.jsx 에서 우리가 렌더링 할 링크카드를 구현해 container/card-container.jsx 에서 데이터 fetching 및 이 녀석들을 렌더링 해주는거죠!

Comment on lines +71 to +99
const formatDate = (dateString) => {
const date = moment(dateString);
return date.format("YYYY.MM.DD");
};

const generateTimeText = (createdAt) => {
const timeDiff = moment().diff(moment(createdAt), "minutes");

if (timeDiff < 2) {
return "1 minute ago";
}
if (timeDiff <= 59) {
return `${timeDiff} minutes ago`;
}
if (timeDiff < 60 * 24) {
const hours = Math.floor(timeDiff / 60);
return hours === 1 ? "1 hour ago" : `${hours} hours ago`;
}
if (timeDiff <= 60 * 24 * 30) {
const days = Math.floor(timeDiff / (60 * 24));
return days === 1 ? "1 day ago" : `${days} days ago`;
}
if (timeDiff <= 60 * 24 * 30 * 31) {
const months = Math.floor(timeDiff / (60 * 24 * 30));
return months === 1 ? "1 month ago" : `${months} months ago`;
}
const years = Math.floor(timeDiff / (60 * 24 * 30 * 12));
return years === 1 ? "1 year ago" : `${years} years ago`;
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

@jisurk
이 경우 card에서만 활용될 요소들이 아닌것으로 보입니다!
/utils.js 라는 파일을 만들어 유틸리티성 함수들을 기록관리하도록 해두면 어떨까요?

Comment on lines +6 to +19
const fetchData = useCallback(async () => {
try {
const response = await fetch(url);
if (response.ok) {
const data = await response.json();
setData(data);
} else {
setData(null);
}
} catch (error) {
console.error("fetch에 실패했습니다.");
setData(null);
}
}, [url]);
Copy link
Collaborator

Choose a reason for hiding this comment

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

@jisurk
음 use-fetch의 경우, 데이터 상태관리까지 처리하면 안될 것 같습니다.
이 요소는 단순히 네트워크 요청 및 에러핸들링 까지의 역할을 해야지, 상태관리까지 하게되면 차후에 더 복잡한 데이터 유형에 대한 관리가 불가능해질 것 같아요.

네트워크 통신 및 response.ok 상태에 따른 에러핸들링을 하는 범위까지만 설정해주세요

font-size: 14px;
`;
function Card() {
const CardData = useFetch("https://bootcamp-api.codeit.kr/api/sample/folder");
Copy link
Collaborator

Choose a reason for hiding this comment

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

@jisurk

잘못 기획된 훅으로 보입니다.
이에 대한 설명은 use-fetch 함수쪽에 설명드렸으니 한번 참고 부탁드려요!

Copy link
Collaborator

Choose a reason for hiding this comment

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

또한 콤포넌트가 아닌 요소에 대해서는 PascalCase가 아닌 cameCase를 사용해주세요

Comment on lines +101 to +107
<StyledUserProfile>
<StyledUserProfileImg
src={Userprofile.profileImageSource}
alt="유저 프로필사진"
/>
<p>{Userprofile.email}</p>
</StyledUserProfile>
Copy link
Collaborator

Choose a reason for hiding this comment

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

@jisurk

profileImg와 email을 보여주는 것을 포함해서 user-profile.jsx라는 콤포넌트를 만드는게 더 의미있어보여요!

);
}

function FolderData() {
Copy link
Collaborator

Choose a reason for hiding this comment

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

@jisurk

한 파일에는 하나의 콤포넌트만 선언해주세요!

}

function FolderData() {
const folderData = useFetch(`${url}/folder`);
Copy link
Collaborator

Choose a reason for hiding this comment

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

@jisurk
훅 개선 필요!

Comment on lines +121 to +132
{folderData && (
<StyledFolderInfo>
<StyledFolderImg
src={folderData.folder.owner.profileImageSource}
alt="프로필"
/>
<StyledFolderOwnerName>
@{folderData.folder.owner.name}
</StyledFolderOwnerName>
<StyledFolderName>{folderData.folder.name}</StyledFolderName>
</StyledFolderInfo>
)}
Copy link
Collaborator

Choose a reason for hiding this comment

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

@jisurk

데이터가 없다면 어떻게 보여질건지도 정의를 해봐야 할 것 같은데, 혹시 시안에는 해당 내용이 없을까요?

Comment on lines 46 to 53
const SearchBarSection = () => {
return (
<SearchBar>
<SearchIcon src={searchIcon} alt="검색 돋보기 아이콘" />
<SearchInput type="search" placeholder="링크를 검색해 보세요." />
</SearchBar>
);
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

@jisurk

한 파일에는 한 콤포넌트에 대한 정의만 해주세요!

@jisurk jisurk added the 매운맛🔥 뒤는 없습니다. 그냥 필터 없이 말해주세요. 책임은 제가 집니다. label Apr 2, 2024
@1005hoon 1005hoon changed the base branch from main to part2-우지석 April 3, 2024 01:39
@1005hoon 1005hoon merged commit 905222f into codeit-bootcamp-frontend:part2-우지석 Apr 3, 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