-
Notifications
You must be signed in to change notification settings - Fork 44
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
[조혜진] Week13 #422
The head ref may contain hidden characters: "part3-\uC870\uD61C\uC9C4-week13"
[조혜진] Week13 #422
Conversation
[Fix] delete merged branch github action
Part3 조혜진 week13
Part3 조혜진 week13
Fix: build error
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.
혜진님 파트3 첫번째 과제 수고하셨습니다 👍🏽
아직 타입스크립트나 next.js 가 익숙하지 않아 어려우셨을텐데 그래도 곧잘 적용해주신 것 같아요
코멘트 참고하셔서 적용 가능한 부분들 시간날 때 수정해보시면 좋을 것 같습니다!
pages/_app.tsx
Outdated
@@ -0,0 +1,17 @@ | |||
// _app.tsx |
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.
의미없는 주석은 삭제 부탁드려요!
public/_redirects
Outdated
@@ -0,0 +1 @@ | |||
/* /index.html 200 |
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.
현재도 필요한 코드인지 확인해보세요!
public/styles/notFound.module.css
Outdated
@@ -0,0 +1,31 @@ | |||
.notFound { |
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.
클래스명을 .index_container
와 같이 snake_case 로 작성한 곳도 있고 지금처럼 camelCase 로 작성한 곳도 있네요
작성 방식을 한가지로 통일해주세요!
src/Components/Cards/Cards.tsx
Outdated
import React from "react"; | ||
import { useState, useRef, useEffect } from "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.
아래처럼 한 줄로 작성할 수도 있습니다!
import React from "react"; | |
import { useState, useRef, useEffect } from "react"; | |
import React, { useState, useRef, useEffect } from "react"; |
src/Components/Cards/Cards.tsx
Outdated
} | ||
}; | ||
|
||
const openModal = (type: "DELETE_LINK" | "ADD") => { |
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.
"DELETE_LINK" | "ADD"
와 같이 타입 정의하는 부분이 여러군데 보이네요
별도 공용 파일에
type ModelType = "DELETE_LINK" | "ADD";
같은 식으로 타입 분리해두고 참조하면 유지보수가 편할 것 같습니다!
보통 공통 타입은 types 라는 폴더 내에서 관리합니다
<Article /> | ||
<section className={styles.section}> | ||
<div className={styles.search_div}> | ||
<Image src='/assets/Search.svg' width='15' height='15' alt='search_icon' /> |
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.
다른 곳들에서는 width, height 를 number 로 전달하셨던데 여기는 string 으로 전달하고 있네요
<div className={styles.search_div}> | ||
<Image src='/assets/Search.svg' width='15' height='15' alt='search_icon' /> | ||
<input | ||
className={`${styles.search_input} input`} |
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.
반복적으로 태그명을 className 에 같이 작성하고 계신데 이유가 궁금합니다!
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.
nextjs로 마이그레이션 이후 reset.css와 global.css에서 태그를 안 쓰고 전역 스타일을 지정하면 오류가 나서 input, button 등의 요소의 초기화 설정을 못 하여서 저렇게 지정해주었습니다...
src/Components/Modal/Modal.tsx
Outdated
if (inputValue.trim() !== "") { | ||
setIsError(false); | ||
} else setIsError(true); |
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.
else 후 중괄호 정확하게 작성해주세요!
if (inputValue.trim() !== "") { | |
setIsError(false); | |
} else setIsError(true); | |
if (inputValue.trim() !== "") { | |
setIsError(false); | |
} else { | |
setIsError(true); | |
} |
<span className={`${styles.subtitle} span`}>{subtitle}</span> | ||
</div> | ||
<div className={styles.submit_container}> | ||
{input && ( |
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.
현재와 같은 구조에서는 프로젝트의 규모가 커지고 모달을 사용하는 곳이 많아질수록 모달 컴포넌트가 담당하는 역할도 같이 늘어나게 됩니다
모달 컴포넌트는 화면 위에 떠있는 껍데기 역할만 하도록 하고
사용처에 따라 모달 내 영역만 다르게 그려서 활용할 수 있도록 하면 모달을 사용하는 곳이 많아지더라도 모달은 본인의 역할을 유지할 수 있을 거에요
당장 코드 개선은 어렵더라도 장기적으로 기능이 계속 늘어날 때에도 괜찮은 구조인가? 에 대해 고민해보시면 좋을 것 같습니다
src/api/parseData.tsx
Outdated
}; | ||
} | ||
|
||
export function SharedData() { |
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.
아까 다른 코멘트에서 함수는 소문자로 시작하는 camelCase 로 작성하라고 말씀드렸는데,
이 부분은 커스텀훅이라고 봐야겠네요.
커스텀훅은 보통 useXX 과 같이 작성합니다 네이밍 변경하는게 좋을 것 같아요!
첨부해주신 링크 (https://5-weekly-mission-r94f4dzjd-hyejeans-projects.vercel.app/) 로 접속이 되지 않아 확인이 불가했습니다 ㅜㅜ
충돌 해결이 안되면 머지가 안되서,, 확인하시고 답변부탁드릴게요! |
요구사항
기본
주요 변경사항
스크린샷
멘토에게