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

Conversation

jinho0941
Copy link
Collaborator

@jinho0941 jinho0941 commented Mar 24, 2024

요구사항

site_link: https://enchanting-torte-57fbbd.netlify.app/

기본

  • 상단 네비게이션 바, 푸터가 랜딩 페이지와 동일한 스타일과 규칙으로 만들어졌나요? (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을 만들어 활용했나요?

주요 변경사항

  • tailwind와 typescript를 추가로 이용하여 구현하였습니다.
  • typescript는 기존 js를 쓰니 컴파일 에러를 잡을수 없다는 부분이 작업 하는데 힘들어서 추가하였습니다. + editor 자동완성
  • skeleton은 작업하면서 패칭하는동안 데이터가 없으니 어색하여 추가했습니다.

스크린샷

image

멘토에게

  • 이번 리뷰도 잘 부탁드립니다.!

@jinho0941 jinho0941 requested a review from 1005hoon March 24, 2024 15:13
@jinho0941 jinho0941 added the 순한맛🐑 마음이 많이 여립니다.. label Mar 24, 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.

@ugaugaugaugaugauga

고생하셨습니다!
이제 html css지옥에 벗어나 리액트로 넘어왔네요 ㅎㅎ
익숙한 라이브러리인만큼 확실히 훨씬 스무스하게 작성된게 보여요

다만 개발 로깅측면에서 그리고 네이밍 관점에서 조금 개선할 부분이 보입니다.
또한 파트1때랑 마찬가지로 폴더 구조들이 너무나도 세분화 되어있다는 부분도 조금 말씀드리고 싶어요!

도메인에 맞춰 모든 요소들을 몰아넣는건 좋지만, 그렇게 몰아넣은 상황에서 더 자세하게 파일별로 구조를 짜는건 개인적으로 추천드리지는 않습니다 ㅠㅠ 지금처럼 구조를 가져간다면 이후에 모든 발생하는 API 콜과 엔드포인트마다 파일이 하나씩 새롭게 만들어져야 하는 형태이잖아요. 따라서 모듈화를 한다고 할 때 특정 도메인 또는 리소스에 활용되는 요소들을 한 파일에 묶어서 관리를 하는 형태로 조금 뭉쳐볼 필요가 있겠습니다.

더 나아가ㅏ, axios를 사용하는 만큼 baseURL 설정해 활용하는 instantiate 기능이 있을텐데요. 이부분도 한번 찾아 적용해보면 좋겠어요!

Copy link
Collaborator

Choose a reason for hiding this comment

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

@ugaugaugaugaugauga

Layout이 홈페이지에서만 활용될 레이아웃인가요? 만약 그렇다면 굳이 홈페이지만을 위한 레이아웃 콤포넌트를 두기 보다 homepage 내부에 레이아웃을 붙여버리면 어떨까요?

Comment on lines +6 to +8
<Layout>
<HomePage />
</Layout>
Copy link
Collaborator

Choose a reason for hiding this comment

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

@ugaugaugaugaugauga

혹시 공통 사용되는 레이아웃은 없으려나요? rrd가 버전 업그레이드 되면서 굉장히 강해진 부분이 outlet을 사용하는 부분이거든요.
또한 App이 가장 최 상단 렌더되는 부분이라는 관점에서, app과 홈페이지가 살짝 구분이 모호해지는게 있는데요.

App은 말 그대로 프로그래밍 차원에서의 기능을 붙이는 작업을 주로 하도록 하구, page 콤포넌트들은 딱 그 페이지에 렌더될 요소들을 처리하는 형태로 말이죠.

따라서 조금 욕심 내본다면

// index.js

const router = createBrowserRouter([
  {
    path: '/',
    element: <App />,
    errorElement: <NotFound />,
    children: [
      { index: true, element: <MainPage /> },
      { path: 'links', element: <LinkPage /> },
      { path: 'links/:id', element: <LinkDetailPage /> },
    ],
  },
])

이런 형태로 두고 App에서는 모든 녀석들이 다 활용할 레이아웃적으로 활용하면서
앱 내에서 사용할 프로바이더들을 설정하는 모양으로 사용하는거죠

/app.tsx

const queryClient = new QueryClient();

function App() {
  return (
    <>
      <AppHeader />
      <LinkProvider>
        <QueryClientProvider client={queryClient}>
          <Outlet />
        </QueryClientProvider>
      </LinkProvider>
    </>
  );
}

export default App;

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

outlet은 nextjs에서 사용하던 layout같은 느낌이군요.
rdd에서는 어떻게 구현하는지 몰라 그냥 이렇게 children을 받아서 적용해 보았는데 outlet을써서 한번 구현해보겠습니다.!

createdAt: string
}

export const Card = ({ id, content, url, createdAt }: Props) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

@ugaugaugaugaugauga

shadcn을 사용하셔서 아시겠지만, Card라는 이름은 사실 UI 콤포넌트중 카드 모양의 경우 활용하는 경우가 많습니다.
따라서 단순히 Card라고 하기보단 조금 더 명세적인 네이밍을 사용하는게 좋겠어요!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

넵 좀더 세부적인 네이밍이 필요할거같내요

)}
<div className='p-5 space-y-3'>
<div className='text-sm text-muted-foreground'>
{getTimeDifference(createdAt)}
Copy link
Collaborator

Choose a reason for hiding this comment

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

@ugaugaugaugaugauga

또한 UI 콤포넌트는 멍청한 콤포넌트로 동작해야한다는 관점에서, 연산 후 보여져야 하는 녀석들에 대한 연산도 부모 콤포넌트에서 미리 다 처리해서 전달하는 형태로 해보면 어떨까요?

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 Author

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.

@ugaugaugaugaugauga

👍🏻

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

👍

const fetchUser = async () => {
try {
setIsLoading(true)
await new Promise((resolve) => setTimeout(resolve, 100))
Copy link
Collaborator

Choose a reason for hiding this comment

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

@ugaugaugaugaugauga

로딩 테스트용 프로미스 제거 필요합니다 ㅋㅋ

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

넵 제거하겠습니다 ㅎㅎ

const user = response.data
setUserData(user)
} catch (error) {
console.error('요청 실패:', error)
Copy link
Collaborator

Choose a reason for hiding this comment

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

@ugaugaugaugaugauga

요청 실패시 사용자에게도 피드백을 주는게 좋겠어요
기획사항에 존재하지 않겠지만 alert또는 다이어로그 콤포넌트정도로 유저에게 피드백을 줄까요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

이부분은 toast로 구현해볼게요

<nav className='z-10 h-20 fixed w-full bg-slate-100 flex items-center justify-between lg:px-36 md:px-20 px-10'>
<img src='/logo.png' alt='logo' />
<div className='flex items-center'>
{isLoading && <Loader className='animate-spin' />}
Copy link
Collaborator

Choose a reason for hiding this comment

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

@ugaugaugaugaugauga

어차피 로더라는 녀석의 목적이 빙글빙글 도는 UI라면 animate-spin 클래스 네임을 외부에서 주입하기보다 그냥 로더에 꽂아버리면 어떤가요?

Comment on lines +42 to +53
{!isLoading &&
(userData ? (
<>
<UserButton url={userData.profileImageSource} />
<p className='ml-3'>{userData.email}</p>
</>
) : (
<Button variant={'primary'} className='rounded-md '>
로그인
</Button>
))}
</div>
Copy link
Collaborator

Choose a reason for hiding this comment

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

@ugaugaugaugaugauga

로직 체크가
로딩상태 체크하고
사용자 정보 존재하는지까지 체크하는 식으로 nested가 되고 있죠

요 부분을 user-account-nav 정도로 콤포넌트 빼서 처리하면 조금 더 깔끔할 것 같은데
어떻게 생각하시나요?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

그게 더 깔끔할거 같내요! 그렇게 해보겠습니다.

Comment on lines 40 to 42
const response = await axios.get<{ folder: Folder }>(
`${import.meta.env.VITE_BASE_URL}/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.

@ugaugaugaugaugauga

보다보니, 네트워크 통신 통해서 받아오는 데이터 response object가 다 다른것 같네요
api 레이어 하나 만들어서 클라쪽에서는 동일한 형태로 무조건 데이터 받도록 처리하는걸 하나 만들어도 좋겠다 싶어요

@1005hoon 1005hoon merged commit 5169359 into codeit-bootcamp-frontend:part2-홍진호 Apr 1, 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