Conversation
There was a problem hiding this comment.
고생하셨습니다.
첫 리뷰인만큼 제 리뷰 스타일을 간단하게 말씀드릴게요
하나의 PR에서 동일한 사항에 대해서는 모든 코드에서 언급하진 않습니다. 만약 네이밍에 대한 리뷰가 있다면 해당하는 코드에 대해서는 자체적으로 검수후 리팩토링을 진행해주시면됩니다.
CSS 구현에 대해서는 거의 보지 않습니다. 정말 이상하다 싶은것이 아니면 CSS에 대한 리뷰는 생략합니다. 만약 어떤 layout이나 ui를 css로 표현해야하는데 어렵다 싶은것이 있다면 별도의 질문으로 남겨주시면 중점적으로 봐드릴수는 있습니다.
"~를 이렇게 한 이유가 있나요?" 라는 질문은 정말로 질문이지 "이 코드 바꿔주세요" 라는 메시지는 아닙니다. 물론 코드만으로 의도를 파악하기 어렵다는 사인이므로, 이유가 타당하지 않다면 "이거 나중에 바꿔주세요" 라는 결과가 될 확률이 높습니다.
전체적으로 크게 문제 없이 잘 하고 계시구요, 몇가지 개선했으면 하는 부분은 아래와 같습니다.
modal 구현관련
- react portal을 이용하여 구현하시는것이 좋습니다.
- 하나의 modal안에서 하는 일이 너무 많습니다. 그 어떤 컴포넌트, 함수, 클래스라도 마찬가지지만, 공통이라고 부르는 모듈내부에 특정 페이지나 데이터에 대한 종속성이 존재하면 안됩니다. react코드가 우리가 작성하는 코드에 대해서는 일절 모르듯이, 우리가 작성하는 코드 중에서도 공통 코드는 특정 유즈케이스에 대한 사항을 알고 있어서는 안됩니다.
style 전용 컴포넌트 규칙
styled component를 사용해서 스타일을 구현하고 계시는데요. 어떨때는 *-Styled.ts 로 분리하다가도 어떨때는 그냥 컴포넌트 내부에 두고 계신데, 뭔가 일관성이 있는 방식으로 개발하시는것이 더 좋을듯합니다.
| @@ -0,0 +1,35 @@ | |||
| import styled from "styled-components"; | |||
|
|
|||
| interface IProps { | |||
There was a problem hiding this comment.
interface 선언시 I- prefix를 붙이는것은 잘 받아들여지는 컨벤션은 아닙니다.
There was a problem hiding this comment.
Prop type을 선언하실때는 ${ComponentName}Props컨벤션을 사용하시는 것이 좋습니다.
type DefaultButtonProps = {};
export default function DefaultButton() {}
| type: "red" | "default"; | ||
| } | ||
|
|
||
| const DefaultBtn = ({ children, onClick, type }: IProps) => { |
There was a problem hiding this comment.
컴포넌트명은 축약어 (Btn)를 이용하지 마시고 명확하게 full name을 사용해주세요 DefaultButton
There was a problem hiding this comment.
또한 DefaultButton이라는 이름 자체가 어색합니다. Button의 기본은 Button입니다.
Button을 확장한 다른 버튼이 나올수 있구요
| interface IProps { | ||
| children: string; | ||
| onClick: (e: React.MouseEvent) => void; | ||
| type: "red" | "default"; |
There was a problem hiding this comment.
red 라는 타입은 너무 변경에 취약합니다.
예를들어 현재 개발할때는 red 가 강조 색상이었는데, 개편 후 blue 가 강조색상이 된다면 코드를 전체적으로 뜯어고쳐야 하는 상황이 나옵니다.
특정 색깔이 아니라 해당 색상이 의미하는 바가 무엇인지를 type으로 받으면 좋습니다.
일반적으로 primary | secondary 와 같은 variant를 사용합니다.
| import styled from "styled-components"; | ||
|
|
||
| interface IProps { | ||
| children: string; |
There was a problem hiding this comment.
button 안에 string만 받기보다는 ReactNode 를 받아야 span태그를 받을수도, svg 아이콘을 받을수도 있습니다.
| background: ${(props) => | ||
| props.type === "red" | ||
| ? "var(--red)" | ||
| : "linear-gradient(91deg, var(--primary) 0.12%, #6ae3fe 101.84%)"}; |
There was a problem hiding this comment.
확장성이 매우 떨어지는 로직이죠?
이외 케이스에는 전혀 대응을 할 수 없습니다.
일단 저라면 아래와 같이 처리할 것 같습니다.
- Button(DefaultButton) 은 기본 시스템 색상 (primary, secondary)에 대한 단색 디자인을 제공한다.
- styled component의 확장 기능을 이용해
GradientButton을 하나 더 만든다.
export const Button = styled.button<{type: 'primary' | 'secondary'}>`
color: ${(props) => `var(--${props.type})`}
`;
export const GradientButton = styled(Button)`
color: linear-gradient(91deg, ${(props) => `var(--${props.type})`}, 0.12%, #6ae3fe 101.84%)
`There was a problem hiding this comment.
@jayjnu
멘토님 궁금한 부분이 있습니다.
현재 버튼이 그라데이션 버튼, 삭제버튼(type:red - 빨간색버튼) 두가지가 존재합니다.
말씀주신 방법에서 따로 type을 받지 않고 기본 color: red로만 처리 후 이를 확장해서 그라데이션 버튼을 만드는 것도 괜찮은가요?
이렇게 하는 이유가 이후에 단색 버튼 색상 추가 혹은 현재 버튼 색상의 변화 등에 대응하기 위해서라고 이해했는데 맞을까요?
아니면 Button을 그라데이션 디자인으로 사용 후 이를 확장해서 DeleteButton(빨간색 버튼)으로 만드는 것이 좋을까요?
디자인에서 기본적으로 사용되는 버튼이 그라데이션 버튼이라 위와 같은 생각이 들었습니다. 혹시 이 방법이 확장성이 떨어지는 형태일까요?
There was a problem hiding this comment.
오늘 말씀드린것처럼 확장에는 열려있되 수정에는 닫혀있게 하기 위함입니다.
만약 추가적인 요구사항이 나올때마다 Button을 수정하거나 새로운 컴포넌트를 만들어야 한다면 확장성이 떨어진다고 볼 수 있겠져
| const ShareFolderModal = () => { | ||
| const { shareFolderModal } = useRecoilValue(modalState); | ||
| const resetModalState = useResetRecoilState(modalState); | ||
| const shareLink = `http://localhost:3000/shared?user=1&folder=${shareFolderModal.content.id}`; |
There was a problem hiding this comment.
localhost:3000은 실제 서비스에서 사용할 수 없는 도메인입니다.
도메인에 대한 값은 환경변수를 이용해서 처리해보세요
https://nextjs.org/docs/pages/building-your-application/configuring/environment-variables
| import KakaoIcon from "@/public/assets/user/img_kakao.png"; | ||
|
|
||
| interface IState { | ||
| state: "signin" | "signup"; |
There was a problem hiding this comment.
하나의 컴포넌트에서 페이지별 분기를 치기보다는 공통되는 요소를 각 페이지별로 가져다가 사용하는 방식이 좋습니다.
여기서 공통요소는 레이아웃이겠죠?
| export default function NotFound() { | ||
| const router = useRouter(); | ||
| const handleBtn = () => { | ||
| router.push("/"); |
There was a problem hiding this comment.
not found페이지가 굳이 router를 사용해서 js를 이용해 동작할 필요가 없습니다.
Button 대신에 Link 를 이용해서 href="/" 로 이동하게 동작하는게 더 효율적입니다.
There was a problem hiding this comment.
@jayjnu
궁금한 부분이 있습니다.
Button 컴포넌트에 onclick props을 내리고 있는데 이 경우에 Button을 따로 onClick을 내리지 않고 Link로 감싸는게 더 좋은가요?
There was a problem hiding this comment.
페이지 네비게이션에 대한 기능을 굳이 Button으로 구현할 필요는 없습니다. 접근성으로나, 기능적인 부분에서 Link가 낫습니다.
아주 LInk를 사용하기 어려운 상황이라고 한다면 Button을 사용해도되지만, 추천하지는 않습니다.
| <Component {...pageProps} /> | ||
| <Script | ||
| src="https://developers.kakao.com/sdk/js/kakao.js" | ||
| onLoad={kakaoInit} |
There was a problem hiding this comment.
모든 페이지에서 카카오톡 sdk를 사용하지 않는데 항상 로드할 필요가 있을까요?
| link: "", | ||
| content: [], | ||
| }, | ||
| shareFolderModal: { |
There was a problem hiding this comment.
modal의 상태를 관리한다면 모달이 열렸는지, 닫혔는지 정도밖에 없어야 하지 않을까 싶습니다.
nesting 될 수 있는 modal이 아니라면 기본적으로 모달은 한번에 한개만 열릴 수 밖에 없고, 각각의 content는 굳이 상태 관리쪽에서는 몰라도 되는 데이터 shape으로 처리할 수 있기 때문이에요
요구사항
기본
‘/folder’ 페이지를 Next.js 프로젝트에 맞게 변경 및 이전 했나요?
‘/shared’ 페이지를 Next.js 프로젝트에 맞게 변경 및 이전 했나요?
다른 페이지로 이동이 필요한 곳에 next/link의 Link를 적용했나요?
Input 컴포넌트에 값이 없는 경우 회색의 placeholder값을 볼 수 있나요?
Input 컴포넌트에 focus in 하면 파랑색 테두리를 볼 수 있나요?
Input 컴포넌트에 눈 모양 아이콘을 누르면 비밀번호 가리기/보기 기능이 토글 되나요?
Input 컴포넌트에 값이 에러케이스일 경우 빨강색 테두리와 에러 메세지를 볼 수 있나요?
Input 컴포넌트에서 focus out 하면 실행할 함수를 설정할 수 있나요?
배포
https://1-weekly-mission-jade.vercel.app/
스크린샷