-
Notifications
You must be signed in to change notification settings - Fork 41
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
feat(app): emotion 기반으로 공용 네비바 로직을 변경합니다. #78
Conversation
9821b25
to
14537db
Compare
<NavList> | ||
<NavItem href="/" isActive title="Home" /> | ||
<NavItem href="/login" title="Sign in" /> | ||
<NavItem href="/register" title="Sign up" /> | ||
</NavList> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 값들을 외부에서 주입 받을 수 있게 한다면, Navbar 컴포넌트는 shared 보다는 상당히 유틸성 높은 컴포넌트가 될 것 같아요. 가령...
type NavbarProps = {
logo?: ReactNode; // 상황에 따라 다릅니다. NavbarBrand에 주입할 수 있을 것 같아요.
menus: {
href: string;
title: string;
}[]
}
export const Navbar: React.FC<NavbarProps> = ({ logo, menus }) => {
return (
<NavbarContainer>
<Container>
<NavbarBrand logo={logo} />
<NavList>
{menus.map(({ href, title }) => (
<NavItem key={title} href={href} isActive title={title} />
))}
</NavList>
// ... 생략 ...
isActive
props를 어떻게 할당할 지는 숙제입니다 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오 감사합니다. 이 코멘트가, 앞서 코멘트 주신 아래 내용과 택일의 사항일 것 같은데, 현재의 realworld 스펙으로는 로고/메뉴 전부 외부 주입이 없어도 되지 않을까 싶은데 어떠신지요?
메뉴 버튼쪽은 누를 때마다 페이지 변환이 되는지라 실질적으로 페이지 내부에서는 해당 값을 바꿔줄 일은 없어 보여서, 새로 페이지가 로딩될 때 해당 페이지의 selected 탭이 어떤 것인지만 타입 같은것으로 전달해주면 어떨까 싶기는 했습니다.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 부분은 PR에 needs-healing 레이블 달아 두고 제가 드린 코멘트와 함께 인성님과 추가 논의 후 추후에 다시 개선해보겠습니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
로그인하면 Navbar가 다른 Route로 변경되는 것으로 알고 있어요!
저도 잘 모르는 입장이지만 이런 경우에도 대응을 하기 위하선 NavBar가 Route를 알지 않고 있는 편이 더 좋지 않을까 생각이 들어요!
NavBar에 일부 경로를 명시해둔다면 이 컴포넌트를 사용하여 다른 경로로 이동이 필요한 시점엔 또 크나큰 고민을 겪게 되지 않을까 싶기 때문이에요~
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
오 로그인 이후 케이스.. 그러네요..! 그걸 전제로 Best Practice 가 어떤 형태인지 또 얘기를 나누어 봐야 겠네요 ㅎㅎ
|
||
export const NavbarBrand = () => { | ||
return ( | ||
<a href="/" style={{ textDecoration: 'none' }}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
인라인 스타일이 들어간 이유가 있을까요?
아마 실수라고 생각되네요 😌
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
인라인 스타일이 들어간 이유가 있을까요? 아마 실수라고 생각되네요 😌
오 네네 제거했습니다!!
07d2697
to
8a746b0
Compare
14537db
to
0f2acf8
Compare
0f2acf8
to
6672475
Compare
📌 이슈 링크
📖 작업 배경
🛠️ 구현 내용
NavBar/
폴더를 만드는게 좋을 듯해 만들고 여기로 이동시켜 주었습니다.💡 참고사항
🖼️ 스크린샷
기존과 동일하게 스타일이 잘 표시되고 있는 상태