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

Conversation

HMRyu
Copy link
Collaborator

@HMRyu HMRyu 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
week7

멘토에게

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

@HMRyu HMRyu requested a review from 1005hoon March 31, 2024 16:51
@HMRyu HMRyu added the 순한맛🐑 마음이 많이 여립니다.. label 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.

@HMRyu

콤포넌트 구조가 생각보다 괜찮습니다!
몇가지 포인트 짚어보자면 fetch 함수의 에러핸들링을 어디서 하면 좋을지, 콤포넌트 구현 시 어디까지 역할과 책임을 전달하면 좋을지, 그리구 화면에서 사용할 데이터의 상태 (null / loading / available) 에 따라 어떻게 처리할건지 등등이 개선되면 좋겠어요.

이에 대한 내용들은 코드단 리뷰에 남겨두었으니 한번 체크 부탁드립니다.
고생 많으셨어요!

Comment on lines +11 to +16
const [user, setUser] = useState({
id: null,
name: null,
email: null,
profileImageSource: null,
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

@HMRyu

각 필드마다 null로 두기 보단, user라는 상태를 nullable로 관리하는게 좋아보여요

Suggested change
const [user, setUser] = useState({
id: null,
name: null,
email: null,
profileImageSource: null,
});
const [user, setUser] = useState(null)

Comment on lines +19 to +38
const loadUserData = async () => {
let result;

try {
result = await fetchUserData();
} catch (error) {
console.error(error);
return;
}

const { id, name, email, profileImageSource } = result;

setUser((p) => ({
...p,
id: id,
name: name,
email: email,
profileImageSource: profileImageSource,
}));
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

@HMRyu

함수가 처리하고 있는 역할이 너무 큽니다!

  1. 데이터 조회하고
  2. 에러핸들링 하구
  3. 상태 업데이트 해주는
    이 모든 역할을 한 함수에 구현되어있는데요.

이 보다는, useEffect 내에서 데이터를 불러와 에러핸들링 및 상태 업데이트를 해주는게 조금 더 좋을 것 같아요.

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
const loadUserData = async () => {
let result;
try {
result = await fetchUserData();
} catch (error) {
console.error(error);
return;
}
const { id, name, email, profileImageSource } = result;
setUser((p) => ({
...p,
id: id,
name: name,
email: email,
profileImageSource: profileImageSource,
}));
}
const loadUserData = async () => {
try {
const result = await fetchUserData();
const { id, name, email, profileImageSource } = result;
setUser((p) => ({
...p,
id: id,
name: name,
email: email,
profileImageSource: profileImageSource,
}));
} catch (error) {
console.error(error);
}
}

Comment on lines +58 to +59
loadUserData();
loadFolderData();
Copy link
Collaborator

Choose a reason for hiding this comment

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

@HMRyu

위 내용대로 업데이트 된다면 이런 형태가 되겠죠?

Suggested change
loadUserData();
loadFolderData();
fetchFolderData()
.then(res => setFolders(res.data))
.catch(e => console.error(e))

Comment on lines +63 to +71
<>
<Navbar user={user} />
<Profile folderData={folderData} />
<Routes>
<Route path="/" element={<CardList folderData={folderData} />}></Route>
<Route path="/detail/:id" element={<Detail />}></Route>
</Routes>
<Footer />
</>
Copy link
Collaborator

Choose a reason for hiding this comment

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

@HMRyu

레이아웃 기능과 라우트 기능이 혼용되어 있는데요. 만약 공통으로 활용되어야 하는 녀석이라면 Outlet이라는 기능을 활용해보는게 좋겠습니다

import SearchImg from '../img/search.svg';
import { calculateTimeDiff, formatDate } from '../utils';

function Card({ link }) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

@HMRyu

한 파일에서는 콤포넌트 하나만 구현해주세요!
또한 Card라는 이름은 카드모양의 UI 콤포넌트인 경우 많이 활용하는 네임이기 때문에
카드 형태인 링크 콤포넌트임을 표현하기 위해 LinkCard 정도의 네이밍이 적합해보입니다!

Comment on lines +33 to +47
<>
<div className='card-container'>
<input className="search-bar" type='text' placeholder='링크를 검색해보세요.'></input>
<img className='search-bar-image' alt='magnifying glass' src={SearchImg} />
<div className="card-list">
{
links.map((link) => {
return (
<Card key={link.id} link={link} />
)
})
}
</div>
</div>
</>
Copy link
Collaborator

Choose a reason for hiding this comment

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

@HMRyu

불필요한 react fragment는 제거되는게 좋겠어요!

</div>
</div>

</>
Copy link
Collaborator

Choose a reason for hiding this comment

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

@HMRyu

react fragment 필요하지 않는 경우엔 제거해주세요!

Comment on lines +5 to +16
function Profile({ user }) {
if (user.email) {
return (
<div className='navbar-profile'>
<img className='navbar-profile-img' alt='profile-img' src={profileImg}></img>
<div className="navbar-user">{user.email}</div>
</div>
)
} else {
return <a className="navbar-login" href="signin.html">로그인</a>;
}
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

@HMRyu

한 파일에는 콤포넌트 하나만 구현해주세요!

또한 이 경우, 사용자 이메일 정보가 없으면 로그인 버튼이 렌더되도록 early return을 활용해보아도 좋겠습니다

Suggested change
function Profile({ user }) {
if (user.email) {
return (
<div className='navbar-profile'>
<img className='navbar-profile-img' alt='profile-img' src={profileImg}></img>
<div className="navbar-user">{user.email}</div>
</div>
)
} else {
return <a className="navbar-login" href="signin.html">로그인</a>;
}
}
function Profile({ user }) {
if (!user.email) {
return <a className="navbar-login" href="signin.html">로그인</a>;
}
return (
<div className='navbar-profile'>
<img className='navbar-profile-img' alt='profile-img' src={profileImg}></img>
<div className="navbar-user">{user.email}</div>
</div>
)
}

Comment on lines +4 to +6
if (!folderData) {
return <p>Loading...</p>
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

@HMRyu

얘두 로딩처리 상위에서 해주세요!

Comment on lines +4 to +6
if (!response.ok) {
throw new Error('정보를 불러오는데 실패했습니다.');
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

@HMRyu

에러를 던진걸 catch를 해주고 있는데, 이런 형태가 될 필요는 없어보여요!
어차피 외부에서 try catch 작업을 해줄거라면 여기선 try catch를 씌울 필요가 없어보입니다

@1005hoon 1005hoon merged commit 7291fde 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