Feature/common layout #16
Conversation
❌ Deploy Preview for prostargram failed.
|
- 새 피드 메뉴 일반피드, 토론 피드로 분리 - 네비게이션바, 메인 css 수
| const Navigation = () => { | ||
| // TODO: 추후 menu url 수정 필요 | ||
| const menus = [ | ||
| const [disscussionModal, setDisscussionModal] = useState(false); |
There was a problem hiding this comment.
모달을 열고 닫는 것에 대한 상태는 하나만 있어도 되지 않을까요?
일반 피드와 토론 피드를 동시에 열고 닫을 경우가 없을 것이라는 생각이 들어서요! 🤔
| @@ -48,13 +99,14 @@ const Navigation = () => { | |||
| return ( | |||
| <li | |||
| className={clsx(styles.menu_item, { | |||
| [styles.active]: activeMenu === idx, | |||
| [styles.active]: activeMenu === idx && menu.type === 'page', | |||
There was a problem hiding this comment.
activeMenu === idx로 배열 인덱스로 활성화 된 메뉴인지를 비교하는 것 보다
usePathname을 통해 현재 페이지의 url과 menu.url이 일치하는지 여부로 확인해보면 어떨까요?
Link API를 통해 각 메뉴에 대해서 클릭 시, 전달된 menu.url로 이동이 될테니 굳이 상태를 쓰지 않고도 가능할 것 같아서요!
참고 링크: Next.js 공식문서 - usePathname
| icon: ReactNode; | ||
| component?: ReactNode; | ||
| modalStatus?: boolean; | ||
| setComponentStatus?: React.Dispatch<SetStateAction<boolean>>; |
There was a problem hiding this comment.
개인적으로는 (React.)Dispatch타입을 사용하고 SetStateAction타입을 import하는 것에 대해서 부정적인 생각을 가지고 있어요!
반드시 사용해야 하는 곳이 아니라면, 왠만한 상황에서는 import하지 않아도 된다고 생각하거든요!
물론.. import 하는 것 자체에 대해서는 문제가 없지만, 불필요한 코드라고 생각해서요 🤔 하하
setComponentStatus의 타입을 (bool: boolean) => void;으로 설정해 두고,
아래와 같이 toggleFeedModal (가명)이라는 함수로써
const toggleFeedModal = (bool: boolean) => {
setFeedModal(bool);
};로만 선언해두고 활용하면 다른 코드들을 모두 대체하면서도, SetStateAction를 import 하지 않아도 되니까요!
| }; | ||
|
|
||
| // TODO: 추후 menu url 수정 필요, 모달 대체 필요 | ||
| const menus: MenuType[] = [ |
There was a problem hiding this comment.
(단순 개인 의견입니다 😅)
MenuType에 대해서 모달이 필요한 ModalMenuType과 MenuType을 분리하면 어떨까요?
현재는 menus라는 하나의 변수안에
일반 메뉴와 피드를 작성해야 하는 메뉴에 대해서 서로 다른 동작을 하는 메뉴가 합쳐져 있어서
type에 따라 조건부로 동작을 달리해야 하니 복잡성이 조금 높다는 생각이 들어서요!
일반 메뉴와 모달을 사용하는 메뉴를 분리하여 별도의 컴포넌트로 만들면,
여기 Navigation에서 props로 받아 따로따로 렌더링 하면 역할도 구분되고
추후 수정 시, 필요한 컴포넌트만 수정하고
다른 동작을 하는 다른 메뉴가 생기면 해당 메뉴에 대한 컴포넌트만 구현해서 추가하면 되니까요!
코드가 늘어난다는 단점이 있지만, 역할 분리와 결합도를 낮추는 측면에서는 도움이 될 것 같은데,
한나님은 어떻게 생각하시는지 궁금합니다! 😉
- 모달 상태 하나로 통일 - usePathname을 사용해 url 일치 여부로 메뉴 활성화 - 모달메뉴와 일반메뉴 분리 - React.Dispatch<SetStateAction<boolean> 타입 제거
|
Pull Request 요약
공통 레이아웃 작업 관련 PR입니다.
작업 내용
참고 사항
관련 이슈