Conversation
There was a problem hiding this comment.
고생하셨습니다.
첫 리뷰인만큼 제 리뷰 스타일을 간단하게 말씀드릴게요
- 하나의 PR에서 동일한 사항에 대해서는 모든 코드에서 언급하진 않습니다. 만약 네이밍에 대한 리뷰가 있다면 해당하는 코드에 대해서는 자체적으로 검수후 리팩토링을 진행해주시면됩니다.
- CSS 구현에 대해서는 거의 보지 않습니다. 정말 이상하다 싶은것이 아니면 CSS에 대한 리뷰는 생략합니다. 만약 어떤 layout이나 ui를 css로 표현해야하는데 어렵다 싶은것이 있다면 별도의 질문으로 남겨주시면 중점적으로 봐드릴수는 있습니다.
- "~를 이렇게 한 이유가 있나요?" 라는 질문은 정말로 질문이지 "이 코드 바꿔주세요" 라는 메시지는 아닙니다. 물론 코드만으로 의도를 파악하기 어렵다는 사인이므로, 이유가 타당하지 않다면 "이거 나중에 바꿔주세요" 라는 결과가 될 확률이 높습니다.
전반적으로 구조를 명확하게 잘 짜신것 같습니다.
styled-components를 사용하시면서 스타일만 적용되어있는 컴포넌트를 별도의 *.style.ts로 분리하신것도 깔끔하고 좋았습니다.
아래는 몇가지 공통적으로 개선하면 좋을 사항들입니다.
1. Fragment 남용
굳이 사용할 필요가 없는데 <></> 로 감싼 코드들이 종종 보입니다. 단일 wrapper만 존재한다면 fragment는 삭제 해주세요
2. Code formatter 미적용
상당부분 code format이 엉성한 코드가 많이 보입니다. 가급적 prettier를 이용해서 일관성있게 코드가 유지될 수 있도록 해주시면 좋습니다.
3. Modal 관련
사실 modal은 이전 팀에서나 지금팀에서나 계속 할말이 나오는 모듈 같은데요, 그래도 혜정님이 짜신 구조가 그나마 가장 확장에 가까운 구조이긴 합니다. 그러나 아직은 공통 코드에 분기들이 들어가고 있습니다. 그부분들만 해소 해주시면 좋을것같습니다.
| :hover { | ||
| background-color : #F0F6FF; | ||
| } | ||
| ` |
There was a problem hiding this comment.
semi colon을 어떤건 붙이셨고 어떤건 안붙이셨는데요, prettier등을 이용해서 잡아주시는걸 추천드릴게요
| <Style.Span>{folder.link.count}개 링크</Style.Span> | ||
| </Style.Box></>))} | ||
| </Style.Container> | ||
| </> |
| } | ||
| `; | ||
|
|
||
| export const P = styled.p` |
There was a problem hiding this comment.
Paragraph 라는 이름을 붙여주는게 더 좋을것같습니다.
| <> | ||
| <Style.Container> | ||
| {folders.map((folder) => | ||
| (<><Style.Box> |
| font-weight: 600; | ||
| letter-spacing: -0.2px; | ||
|
|
||
| span { |
There was a problem hiding this comment.
너무 특정상황에만 적용되는 스타일입니다.
차라리 Keyword 처럼 별도의 styled component를 하나 더 만들고 재사용하는게 훨씬 명시적이지 않을까싶습니다.
| import deleteIcon from "@/src/assets/Delete.svg"; | ||
|
|
||
| const Icons = [ | ||
| { name: "공유", image: shareIcon, option: { title: "폴더 공유", trigger : "ShareOnSns" } }, |
There was a problem hiding this comment.
각 요소가 될 수 있는 타입을 지정해주시면 더 스트럭쳐가 있는 데이터를 관리하기 좋지 않을까싶네요
| import penIcon from "@/src/assets/pen.svg"; | ||
| import deleteIcon from "@/src/assets/Delete.svg"; | ||
|
|
||
| const Icons = [ |
There was a problem hiding this comment.
PascalCase 는 언제나 class또는 component에만 사용해주세요
|
|
||
| export default function App({ Component, pageProps }: AppProps) { | ||
| return <Component {...pageProps} /> | ||
| const [isClient, setIsClient] = useState(false); |
There was a problem hiding this comment.
서버사이드 html과 일치하지 않아서 hyration할 수 없다는 오류가 나면서 화면이 무한랜더링? (style-component를 넣은 document와 연관이 있을지는 잘 모르겠지만)이 되어서, 일단 way to fix it 에서 설명해주는대로 위와같이 넣었더니 오류는 사라졌습니다.
| <ThemeProvider theme={theme}> | ||
| <GlobalStyle /> | ||
| <Nav /> | ||
| <footerContext.Provider value={footerRef}> |
There was a problem hiding this comment.
왜 ref를 상단에서 알고있어야 할까요? 이게 isClient로 분기가 필요한 이유인가요?
There was a problem hiding this comment.
footer ref를 사용해 intersectionObserver의 대상으로 사용하고 싶었는데 그러기 위해 _app에서 사용되는 footer의 ref를 내려주기 위해 전역컨텐트를 만들어 내려줬습니다.
There was a problem hiding this comment.
ref가 아니라 footer에서 isVisible 상태를 전역으로 공유하면 어떨까요?
Footer.tsx
export default function Footer() {
const {isVisible, ref} = useIntersectionObserver();
const {setIsVisible} = useFooterDispatch();
useEffect(() => {
setIsVisible();
}, [isVisible])
return <footer ref={ref}></footer>
}Header.tsx
export default function Header() {
const footer = useFooterState();
if (footer.isVisible) {
///
}
}_app.tsx
export default function App({Component, ...props}) {
return <FooterStateProvider><Component {...props}/></FooterStateProvider>
}| @@ -0,0 +1,13 @@ | |||
| import { useState } from "react"; | |||
|
|
|||
| function useModal () { | |||
There was a problem hiding this comment.
useModal이 하는일은 결국 useState와 다를바가 없어보입니다.
불필요한 추상화가 아닐까 싶네요
멘토에게