-
Notifications
You must be signed in to change notification settings - Fork 14
[김제성] sprint 9 #75
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
base: main
Are you sure you want to change the base?
[김제성] sprint 9 #75
The head ref may contain hidden characters: "next-\uAE40\uC81C\uC131-sprint9-new"
Conversation
rl0425
left a comment
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.
고생하셨습니다.
다만 코드를 작성하실때, 한 컴포넌트에 너무 많은 코드와 중첩된 요소를 사용하게 되면 가독성이 떨어지게 됩니다. 이는 다른 개발자가 볼 때 뿐 아니라, 본인이 유지 보수하기에도 힘들 있기 때문에 이 점 참고하셔서, 작성하실 때 어떻게 더 알아보기 쉽게 코드를 작성할 수 있을까를 생각하시면 더 좋은 개발자가 되실 것 같습니다.
지금 문제가 있어서 말씀드리는게 아니라, 추후 더 복잡한 코드 개발 시에 팁이라고 생각해주시면 될 것 같습니다.
현재 공통 컴포넌트를 만드시는건 너무 잘하고 계시고, 리뷰에 남겨드린 children 요소 삽입이나 기본값 설정만 주의하시면 될 것 같습니다. 또 next의 Link, Image 사용도 자연스럽게 잘 사용하고 계십니다.
| export default function BestPosts({ className = "", posts }) { | ||
| const router = useRouter(); | ||
|
|
||
| const handlePostClick = (postId) => { |
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.
링크를 임포트해서 사용하셔서 이 펑션은 사용하시지 않는 것 같아, 라우터 코드와 함께 지우셔야 할 것 같습니다.
| <div className={`${styles.bestPosts} ${className}`}> | ||
| <h2 className={styles.title}>베스트 게시글</h2> | ||
| <div className={styles.postGrid}> | ||
| {posts?.map((post) => ( |
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.
posts를 ?로 체킹하시는건 좋지만, 막상 포스트가 없을 경우에 띄울 내용이 없습니다. 때문에
posts && <현재 컴포넌트>
!posts && <비었을 경우 컴포넌트>위 코드처럼 &&을 사용하시거나, 삼항 연산자를 사용하셔서 비었을 경우도 만들어 주시면 좋을 것 같습니다.
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.
밑에 post.views, post.date도 마찬가지로, 해당 데이터가 없을 경우의 예외처리를 같이 해주셔야 합니다.
ex)
{post.title || "제목 없음"}| @@ -0,0 +1,7 @@ | |||
| import styles from "./Button.module.css"; | |||
|
|
|||
| export default function Button({ className = "", as, ...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.
공통 컴포넌트를 만드시는거라면 Button 같은 경우에는 열린 태그이기 때문에, 안쪽 요소를 받을 수 있어야 합니다. 보통은 children default props를 사용해서 내부 데이터를 표현합니다.
function Button({ className = "", as, children, ...props }) {
...
return <button className={`${buttonStyles.button} ${className}`} {...props}>{children}</button>;| const classNames = `${styles.input} ${ | ||
| isOpen ? styles.opened : "" | ||
| } ${className}`; | ||
| const selectedOption = options.find((option) => option.value === value); |
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.
옵션에서 벨류를 못 찾을 경우의 예외처리가 필요합니다.
| import { useEffect, useState, useRef } from "react"; | ||
| import styles from "./Dropdown.module.css"; | ||
|
|
||
| export default function Dropdown({ |
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.
공통 드랍다운 컴포넌트면, 접근성을 챙기기 위해서 ARIA 속성과 키보드 네비게이션도 추후에 지원하시는걸 추천드립니다.
| {selectedOption.label} | ||
| <span className={styles.arrow}>▴</span> | ||
| <div className={styles.options}> | ||
| {options.map((option) => { |
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.
jsx 안에서 조건문이나 변수 사용을 하시기 보다는, 따로 컴포넌트를 파시고 컴포넌트를 호출하시는 방법을 더 추천드립니다.
| @@ -0,0 +1,30 @@ | |||
| const { createContext, useContext, useState, useEffect } = require("react"); | |||
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.
혹시 require을 쓰신 다른 이유가 있으실까요? 없으시면 ES6 문법인 import를 사용하시는게 통일성이 있을 것 같습니다.
| @@ -0,0 +1,30 @@ | |||
| const { createContext, useContext, useState, useEffect } = require("react"); | |||
|
|
|||
| export const ThemeContext = createContext(); | |||
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.
Context를 사용하실 땐 초기값을 설정해주시는게 좋습니다!
export const ThemeContext = createContext({
theme: "dark",
setTheme: () => {},
});
요구사항
기본
심화
주요 변경사항
스크린샷
멘토에게