Skip to content
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

[feat]MyPage React #88

Open
wants to merge 1 commit into
base: jinmo
Choose a base branch
from
Open

Conversation

WiFiHan
Copy link

@WiFiHan WiFiHan commented Jun 1, 2023

I am in trouble pushing "Django" git.

Copy link

@whwoohw whwoohw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

고생하셨습니다! 수정할만한 부분들 comment 남겨놓았으니 참고하고 수정해주세요!
또, email, username, major, college 부분이 모두 동일한 형태이니 컴포넌트화를 시켜줘도 괜찮겠죠??

Comment on lines +6 to +9
const [isEmailEdit, setIsEmailEdit] = useState(false);
const [isUsernameEdit, setIsUsernameEdit] = useState(false);
const [isCollegeEdit, setIsCollegeEdit] = useState(false);
const [isMajorEdit, setIsMajorEdit] = useState(false);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 부분의 경우 formData를 처리하는 것처럼 한번에 관리해주는게 좋아요!

Suggested change
const [isEmailEdit, setIsEmailEdit] = useState(false);
const [isUsernameEdit, setIsUsernameEdit] = useState(false);
const [isCollegeEdit, setIsCollegeEdit] = useState(false);
const [isMajorEdit, setIsMajorEdit] = useState(false);
const [isEdit, setIsEdit] = useState({
email : false,
username: false,
college: false,
major: false
})

이런식으로요!

Comment on lines +30 to +45
const onEmailEditClick = async (e) => {
setIsEmailEdit(true);
};

const onUsernameEditClick = async (e) => {
setIsUsernameEdit(true);
};

const onCollegeEditClick = async (e) => {
setIsCollegeEdit(true);
};

const onMajorEditClick = async (e) => {
setIsMajorEdit(true);
};

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

우선, 이 부분은 그냥 setState를 활용해서 state를 변경해주는 것이기 때문에 비동기 처리가 아니예요! async를 사용할 필요가 없습니다.
그리고 e를 사용하지 않는다면 굳이 적어둘 필요 없습니다!

또, 이 부분도 isEdit으로 한번에 관리한다면 훨씬 편하겠죠?
만약에 button의 id를 email, username 이런식으로 설정한다고 가정한다면

Suggested change
const onEmailEditClick = async (e) => {
setIsEmailEdit(true);
};
const onUsernameEditClick = async (e) => {
setIsUsernameEdit(true);
};
const onCollegeEditClick = async (e) => {
setIsCollegeEdit(true);
};
const onMajorEditClick = async (e) => {
setIsMajorEdit(true);
};
const onEditClick = (e) => {
const { id } = e.target
setIsEdit((prev) => ({...prev, [id] : true })
};

이런식으로요!

Comment on lines +21 to +28
const onSubmit = async (e) => {
e.preventDefault();
await updateMyInfo(formData);
setIsEmailEdit(false);
setIsUsernameEdit(false);
setIsCollegeEdit(false);
setIsMajorEdit(false);
};
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updateMyInfo api에 새로고침을 하는 부분이 없는것 같은데, 새로고침이 됐나요??
그리고 만약에 새로고침을 한다면 뒤에 setState는 의미가 없습니다! 어차피 새로고침 하니 알아서 전부 초기 상태로 돌아가기 때문이죠

if (getCookie("access_token")) {
const getMyPageAPI = async () => {
const userProfile = await getMyPage();
setUserProfile(userProfile);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

userProfile을 받아올 때 아마 major, college, user 이렇게 나눠져 있고, user 안에 username과 email이 있을 거예요. 굳이 이 상태로 쓸 필요는 없겠죠?
저라면 필요한 값들만 넣어줄 것 같습니다.

Suggested change
setUserProfile(userProfile);
setUserProfile({
email : userProfile.user.email,
username : userProfile.user.username,
major : userProfile.major,
college : userProfile.college,
});

이런 식으로요!

그리고 userProfile 변수가 중복 사용된 것 같은데, 아마 문제는 없었을 것 같은데 그래도 최대한 변수명은 다르게 설정해주는게 좋아요!

Comment on lines +55 to +63
<div className="flex w-full gap-x-5">
<input
type="text"
placeholder="Add Tags.."
id="email"
value={formData.user.email}
className="input grow"
onChange={handleChange}
/>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

이 부분도 isEdit이 true면 input이지만, isEdit이 false면 그냥 텍스트로 보여지도록 설정해야 할 것 같습니다!

const [isUsernameEdit, setIsUsernameEdit] = useState(false);
const [isCollegeEdit, setIsCollegeEdit] = useState(false);
const [isMajorEdit, setIsMajorEdit] = useState(false);
const [formData, setFormData] = useState(content);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

굳이 content를 그대로 사용하지 않고, formData에 넣어준 이유가 있을까요?

Comment on lines +12 to +19
const handleChange = (e) => {

if (e.target.id === "email" || e.target.id === "username")
{
setFormData({ ...formData, [`user`]: {...formData.user, [e.target.id]: e.target.value} });
}
else setFormData({ ...formData, [e.target.id]: e.target.value });
};
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MyPage 부분에 남긴 comment를 참고한다면 이 부분도 보다 간단하게 바꿀 수 있을거예요!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants