-
Notifications
You must be signed in to change notification settings - Fork 39
[박다인] sprint6 #202
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 #202
The head ref may contain hidden characters: "React-\uBC15\uB2E4\uC778-sprint6"
Conversation
…ithub-actions [Fix] delete merged branch github action
…인-sprint1 [박다인] Sprint1
…다인-sprint2 [박다인] Sprint2
…다인-sprint3 [박다인] sprint3
addiescode-sj
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.
수고하셨습니다!
주요 리뷰 포인트
- 상태 객체 묶음 작게 분리해서 리렌더링 횟수 줄이기
- 인라인 함수 복잡도 줄이기, 코드 중복 제거하기
- 에러 상태 개선하기
| const fileInputRef = useRef(null); | ||
|
|
||
| const handleInputChange = (field, value) => { | ||
| setNewItem((prev) => ({ ...prev, [field]: 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.
React의 Batch 업데이트 특성을 생각해서 이렇게 처리하신건가요? 굳굳!! 👍
| onChange={(e) => { | ||
| if (newItem.images) { | ||
| setImageAlert(`*이미지 등록은 최대 1개까지 가능합니다.`); | ||
| return; | ||
| } | ||
| handleInputChange("images", e.target.files[0]); | ||
| setImageAlert(""); // 정상 등록이면 에러 제거 | ||
| }} | ||
| /> |
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 validateImageUpload = () => {
if (newItem.images) {
setImageAlert(`*이미지 등록은 최대 1개까지 가능합니다.`);
return;
}
}| const [newItem, setNewItem] = useState({ | ||
| images: null, | ||
| tagInput: "", | ||
| tags: [], | ||
| price: "", | ||
| description: "", | ||
| name: "", | ||
| }); |
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 state를 다루고있네요!
상태 객체가 너무 큰 묶음으로 관리되다보면 불필요한 리렌더링이 자주 발생될거예요.
아래와 같이 관련된 상태끼리 묶어서 분리해볼까요?
const [formData, setFormData] = useState({
name: "",
description: "",
price: "",
});
const [tags, setTags] = useState([]);
const [tagInput, setTagInput] = useState("");
const [images, setImages] = useState(null);| className="item__delete" | ||
| onClick={() => { | ||
| handleInputChange("images", null); | ||
| setImageAlert(""); |
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 [errors, setErrors] = useState({
image: "",
name: "",
description: "",
price: "",
tags: ""
});| newItem.tags.length > 0; | ||
|
|
||
| return ( | ||
| <> |
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.
여기 fragment가 필요없습니다 :)
질문에 대한 답변
CRA로 진행하셨으면 .env파일 만들고 환경변수 설정 시 앞에 REACT_APP_ 붙여주시는게 맞습니다! |
요구사항
기본
심화
주요 변경사항
스크린샷
멘토에게