-
Notifications
You must be signed in to change notification settings - Fork 39
[송시은] Sprint6 #203
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
[송시은] Sprint6 #203
The head ref may contain hidden characters: "React-\uC1A1\uC2DC\uC740-sprint6"
Conversation
GANGYIKIM
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.
시은님 6번째 미션 작업 고생하셨습니다!
다음 미션도 화이팅입니다~
-
지난번 말씀하신 이 부분은 아이템 페이지일 경우 네비게이션 중고 마켓이 active되서 파란글씨가 되야해서 사용했는데 괜찮을까요?:
React router 라이브러리에서 Link 컴포넌트가 wrapping된 NavLink를 제공합니다. 이런 경우 해당 컴포넌트를 사용하시는 것이 더 적절한 방법입니다! -
말씀하신대로 hook이름을 명확히했습니다. 현재는 다른 훅도 추가되었기 때문에 훅이 여러개인 경우 각각의 사용처에 hooks/를 생성하면 여러 hooks/가 반복 생성되서 src/hooks/에 그냥 두었는데 괜찮을까요?:
관련해서 코멘트에도 남겨드렸습니다. 간략히 얘기하자면 src 밑에 있는 hooks들을 프로젝트 전반에서 공용으로 사용되는 경우에 해당 폴더에 위치시키시는 것이 좋고 특정 페이지, 컴포넌트에서만 사용되는 경우 사용처에 폴더를 생성해서 위치시키시는 것이 구조 파악 및 관리 측면에서 유리합니다. -
말씀하신 것 처럼 각 route에 따라 layout를 구성하면 아래처럼 수정하는게 맞을까요? 근데 이 방식이 지금 사용하는 방식보다 더 복잡하게 느껴지는데 강사님 의견이 궁금합니다.:
코멘트를 비판적으로 수용하시는 점 좋습니다~ Layout 을 적용하지 않은 현 구조의 경우 Header 컴포넌트는 페이지가 마운트, 언마운트될때마다 같이 포함되게 되며, 어떤 페이지들이 같은 레이아웃을 공유하는지 알기가 어렵습니다. 그래서 예시 코드처럼 레이아웃 컴포넌트로 빼, 페이지별 레이아웃을 공용으로 관리해주시는 것이 좋습니다.
| @@ -1 +1,6 @@ | |||
| export const baseUrl = import.meta.env.VITE_BASE_URL; | |||
|
|
|||
| export const ENDPOINTS = { | |||
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.
💊 제안
상수 네이밍시 단어를 구분해주시는 것을 추천드려요!
| export const ENDPOINTS = { | |
| export const END_POINTS = { |
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.
💊 제안
이렇게 컴포넌트로 분리하신 이유를 모르겠어요.
기존의 코드들을 하나의 컴포넌트로 단순히 분리만 한거라 구조를 파악하고 싶을때 하나의 파일을 더 봐야해서 가독성에도 좋지 않은 것 같아요.
만약 route 파일을 따로 빼고 싶으시다면 지금은 react-router-dom 라이브러리의 declative 모드로 작업하고 계시는데 Data 모드나 Framework 모드를 사용하시는 것이 더 적절할 것 같아요~
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.
💊 제안
해당 코드는 이름과 내용면에서 addItem이라는 페이지 종속적이라는 생각이 들어요.
이런 경우 근처에 위치시키는 것이 더 적절해보입니다~ 각 페이지에 종속적인 로직이 있따면 pages 폴더 하위 각 페이지 폴더에 utils, hook등을 만들어 위치시키시는 것을 추천드려요.
| <input | ||
| id="productImg" | ||
| type="file" | ||
| accept="image/*" |
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.
💊 제안
input의 accept 속성은 유저가 어떤 파일을 올려야하는지에 대한 힌트를 제공하는 속성입니다.
유저는 파일 업로드시 accept의 명시된 확장자 이외의 파일도 올릴 수 있으므로
실제 upload 함수에서 한번더 확장자를 검사해주시는 것이 좋습니다.
(사용자가 업로드창에서 옵션을 열어 확장자를 바꾸면 아래처럼 보입니다)

https://developer.mozilla.org/en-US/docs/Web/API/HTMLInputElement/accept
| handleInputChange, | ||
| imagePreview, | ||
| setImagePreview, | ||
| showImageWarning, |
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.
💊 제안
boolean 타입임을 알 수 있게 isShowImageWarning 같은 이름을 추천드려요.
일반적으로 React에서 동사로 시작하는 이름들은 함수를 의미합니다~
| import formStyles from '@/styles/helpers/formHelpers.module.scss'; | ||
| import styles from './TagInput.module.scss'; | ||
|
|
||
| const TagInput = ({ tagInput, setTagInput, tags, handleInputChange }) => { |
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.
💊 제안
props명과 컴포넌트 명들이 중복되는 정보들을 가지고 있어요.
아래처럼 작성하셔서 좀 더 명확하게 네이밍하시는 것을 추천드려요!
| const TagInput = ({ tagInput, setTagInput, tags, handleInputChange }) => { | |
| const TagInput = ({ tags, handleInputChange }) => { |
또한 setTagInput와 handleInputChange를 둘 다 받고 있습니다.
지금 제가 파악하기로는 tagInput과 같은 인풋값이 외부에서 필요한 것이 아니라서 컴포넌트 내부에서 처리하는 것이 좋아 보입니다.
| @@ -0,0 +1,14 @@ | |||
| import styles from './RemoveIcon.module.scss'; | |||
|
|
|||
| const RemoveIcon = ({ onClick, 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.
💊 제안
버튼이라는 것을 알 수 있는 이름이면 좋을 것 같아요!
|
|
||
| const ToastContext = createContext(); | ||
|
|
||
| export const useToast = () => useContext(ToastContext); |
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를 사용하는 훅을 정의하셔서 사용성을 높으신 것 좋습니다.
다만 아래처럼 context 내부에서 해당 context에 접근하지 않을때의 에러도 추가해주시면 더 좋을 것 같습니다!
| export const useToast = () => useContext(ToastContext); | |
| export const useToast = () => { | |
| const _context = useContext(ToastContext); | |
| if (!_context) throw new Error(/** error msg */); | |
| return _context; | |
| } |
| </button> | ||
| </div> | ||
|
|
||
| <form className={formStyles.form}> |
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.
💊 제안
form 요소를 사용하고 계시니 등록 버튼을 form과 연결하셔서 form의 onSubmit 이벤트로 핸들링 하시는 것을 추천드려요!
버튼이 form 외부에 위치해 있어도 form의 id를 지정하고 form 속성으로 연결할 수도 있습니다~
| <form className={formStyles.form}> | |
| <form className={formStyles.form} onSubmit={handleSubmit}> |
이렇게 해주셔야 form 내부에서 enter 키 입력으로 제출 이벤트가 발생되었을 시 handleSubmit이 실행됩니다!
| <div className={styles.header}> | ||
| <h2>상품 등록하기</h2> | ||
| <button | ||
| type="button" |
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.
❗️ 수정요청
등록 버튼은 영역을 제출하는 것이므로 submit type이 적절합니다.
| type="button" | |
| type="submit" |
요구사항
배포 링크: https://codeit-sprint15-mission.netlify.app/
기본
상품 등록
심화
상품 등록
이번 미션 요구사항은 아니지만 추가한 부분
주요 변경사항 (지난 미션 부분 수정)
.removeIcon--image와 같이--modifier로 스타일을 적용했었는데 지난 PR 강사님의 피드백을 보고 생각해보니 removeIcon처럼 특정 기능을 가진 단일 UI 요소는 컴포넌트 내부에서 className prop 조합으로 관리하는 방식이 더 나아 보여서 컴포넌트 + className prop 으로 변경했습니다..primary라고 클래스를 정의해 사용하는 쪽으로 변경했습니다. 이렇게 하니까 기존에는 button과 버튼태그를 사용하지 않지만 버튼처럼 쓰는 경우는.button으로 사용했었는데.primary하나로 간단해 지는 효과도 있었습니다.price === 0→ "나눔"price가undefined또는null→ "가격 미정"toLocaleString()사용스크린샷
멘토에게
지난번 말씀하신 이 부분은 아이템 페이지일 경우 네비게이션 중고 마켓이 active되서 파란글씨가 되야해서 사용했는데 괜찮을까요?
말씀하신대로 hook이름을 명확히했습니다. 현재는 다른 훅도 추가되었기 때문에 훅이 여러개인 경우 각각의 사용처에 hooks/를 생성하면 여러 hooks/가 반복 생성되서 src/hooks/에 그냥 두었는데 괜찮을까요?
말씀하신 것 처럼 각 route에 따라 layout를 구성하면 아래처럼 수정하는게 맞을까요?
근데 이 방식이 지금 사용하는 방식보다 더 복잡하게 느껴지는데 강사님 의견이 궁금합니다.