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

Feature : 공통컴포넌트 - 헤더 및 레이아웃 작업 #5

Merged
merged 14 commits into from
Aug 4, 2023

Conversation

seondal
Copy link
Member

@seondal seondal commented Aug 2, 2023

💡 왜 PR을 올렸나요?

  • 신규 피처

💁 무엇이 어떻게 바뀌나요?

  1. 헤더가 추가되었습니다
  2. 디자인 시스템에 따라 컬러들을 관리합니다. (figma와 컬러 이름 맞춤)
  3. 탭을 눌러서 각 기능에 맞는 페이지로 라우팅이 가능합니다

📆 작업 예정인 것이 있나요 ?

  • 포즈피드

💬 리뷰어분들께

  • Featuring : 초기 세팅 #4 전 PR에 제가 추가한 세팅 관련 변경사항이 있으니 참고해주세요!
  • 슬랙에도 보냈는데, pxToRem 의 제거에 대한 논의가 필요할 것 같습니다!

@seondal seondal self-assigned this Aug 2, 2023
@seondal seondal requested a review from guesung as a code owner August 2, 2023 17:57
@seondal seondal added ✨ Feat 새로운 기능 개발 💄 Style 화면 그리기, 스타일링 labels Aug 2, 2023
Copy link
Collaborator

@guesung guesung left a comment

Choose a reason for hiding this comment

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

고생하셨습니다 ~

리뷰 한번씩 확인 부탁해요 !

혹시 긴히 논의할 사항이 있으면 슬랙 DM으로 언제든지 말씀해주세용

{children}
<body className="flex h-[100dvh] w-screen touch-none justify-center bg-slate-100 py-px">
<div className="max-w-440 h-full w-full bg-white py-2 text-black drop-shadow-2xl">
<Header />
Copy link
Collaborator

Choose a reason for hiding this comment

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

여기에 <Header />를 두게되면 모든 페이지에 공통으로 이 <Header />가 적용이 될거에요.여기에 두는 것보다는 각 폴더의 layout에 두는 것이 더 적합하다고 생각해요 !

Copy link
Member Author

@seondal seondal Aug 3, 2023

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.

최상단 layout.tsx는 현재 server component여서 pathname을 받아오지 못합니다.

따라서, path에 따라 다른 Header를 보여줄 수 없습니다.

leerob.io와 같은 방식으로 Header에서 다르게 보여주는 방법도 있지만, 저희는 포즈픽,톡,피드 외의 다른 페이지와 Header구성이 전혀 다르기에 적합한 방식이라 생각하지 않습니다.

묶어서 layout을 적용할 수 있는 Route Handler를 이용하면 좋을 것 같습니다. 이 부분은 제가 한 번 해볼게요 !

src/app/layout.tsx Outdated Show resolved Hide resolved
src/components/header/index.tsx Outdated Show resolved Hide resolved
styles/font.css Show resolved Hide resolved
styles/theme/colors.ts Show resolved Hide resolved
src/components/header/Tab.tsx Show resolved Hide resolved
src/components/header/Tab.tsx Outdated Show resolved Hide resolved
Comment on lines 19 to 28
<div key={item.path}>
<div className="py-3">
<h5 className="text-brand">{item.title}</h5>
</div>
<div className="border-b-main-violet relative top-0.5 border-b-2" />
</div>
) : (
<Link href={item.path} className="py-3" key={item.path}>
<h5 className="text-secondary">{item.title}</h5>
</Link>
Copy link
Collaborator

Choose a reason for hiding this comment

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

현재 py-3, key={item.path}코드가 중복되고 있습니다.

이러한 코드를 묶어주면 더 가독성이 있고, 유지보수 측면에서도 좋을 것 같아요.

Copy link
Collaborator

Choose a reason for hiding this comment

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

{tabData.map((item) => (
  <div key={item.path} className="py-3">
    {item.path === path ? (
      <h5 className="text-brand">{item.title}</h5>
    ) : (
      <Link href={item.path}>
        <h5 className="text-secondary">{item.title}</h5>
      </Link>
    )}
  </div>
))}

제가 짜본 코드입니다. 참고 부탁해요 !

Copy link
Member Author

Choose a reason for hiding this comment

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

tailwind는 중복 처리에 불리하다고 생각했는데 규성님처럼 하면 해결되겠군요..! 감사합니다 적용하겠습니다!

styles/typography.css Show resolved Hide resolved
@guesung
Copy link
Collaborator

guesung commented Aug 3, 2023

또한, 저희 commit할 때 이모지 넣는 것에 대해서도 논의가 필요해 보여요 !

@seondal seondal requested a review from guesung August 4, 2023 10:28
@guesung guesung merged commit 23b4d0f into develop Aug 4, 2023
@guesung guesung deleted the feature/OZ-41-tab branch August 4, 2023 11:09
@seondal seondal added this to the DND Final Project ⛳️ milestone Aug 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✨ Feat 새로운 기능 개발 💄 Style 화면 그리기, 스타일링
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants