Feat: Header 컴포넌트 구현#53
Conversation
jyeon03
left a comment
There was a problem hiding this comment.
헤더 구현 수고하셨어요~!~!!
리뷰 요청과 관련해서 이야기해보자면 상태에서 하나의 Header 컴포넌트에 variant로 합치면 variant === "home" / variant === "back"에 따라서 필요한 props와 렌더링 구조가 크게 달라져서 오히려 조건 분기가 많아질 것 같아요!! 그래서 지금처럼 역할 기준으로 컴포넌트를 분리하는 게 더 읽기 쉽고 유지보수에도 좋을 것 같아욤!! 🥰
There was a problem hiding this comment.
HomeHeader가 홈에서만 쓰이는 헤더라면 shared/ui/header보다 features/home/components 쪽에 두는 게 맞을 것 같아요!! 메뉴 구성이나 유저 버튼까지 홈 화면에 강하게 묶여 있어서 완전한 shared ui라기보다는 home feature 컴포넌트에 가까운 것 같아요!
There was a problem hiding this comment.
HomeHeader shared/ui/header->features/home/components 로 이동했습니다~!
chungyo
left a comment
There was a problem hiding this comment.
고생하셨어요~~~~~ 코드 확인 잘 했고 전체적으로 좋았지만 명세서 기준으로 확인이 필요한 부분이 있어 코멘트 남겼습니다!! 확인해주시고 반영 부탁드려요!!
| className={cn("bg-snapdeck-000", "z-(--z-header)", "h-[4.7rem] px-16")} | ||
| > | ||
| <div className="flex h-full items-center justify-between pt-[0.9rem]"> | ||
| <LogoImg className="size-[3rem]" /> |
There was a problem hiding this comment.
명세에는 로고가 “홈으로 복귀할 수 있는 진입점”으로 되어 있는데, 현재는 LogoImg만 렌더링되어 있어서 클릭 동작이나 접근 가능한 이름은 없는 상태인 것 같아요.
이번 범위에서 실제 이동 동작을 제외하는 게 맞다면 괜찮지만, 로고를 홈 이동 진입점으로 볼 예정이라면 추후 button 또는 Link로 감싸고 aria-label을 추가해주면 좋을 것 같습니다..!!!
| const HomeHeader = () => { | ||
| return ( | ||
| <header | ||
| className={cn("bg-snapdeck-000", "z-(--z-header)", "h-[4.7rem] px-16")} |
There was a problem hiding this comment.
이 부분도 화면 명세에는 상단 GNB가 화면 최상단에 고정 노출되는 구조로 적혀 있는데, 현재 HomeHeader는 z-index만 있고 fixed나 sticky 속성은 없는 것 같아요!!
스크롤 시에도 헤더가 고정되어야 하는 요구사항이라면 sticky top-0 또는 fixed top-0 적용 여부를 확인해보면 좋을 것 같습니다!
📌 Summary
Header 컴포넌트를 구현했습니다!
📚 Tasks
🔍 Describe
HomeHeader, BackHeader를 구현했습니다.
shared/ui/header/index.ts barrel export를 추가했습니다.👀 To Reviewer
Header를 variant 구조로 합치기보다 분리하는 방향이 적절한지 의견 부탁드립니다!
HomeHeader를 Figma 구조에 최대한 맞추려고 하다 보니 layout 구조가 조금 복잡해진 것 같은데 더 깔끔하게 개선할 수 있는 부분이 있다면 리뷰 부탁드립니다 🙇🏻♀️
📸 Screenshot