-
Notifications
You must be signed in to change notification settings - Fork 0
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
[4주차 기본/심화/공유 과제] API 통신 #5
base: main
Are you sure you want to change the base?
Conversation
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.
GlobalStyle과 Themes를 세세하게 잘 활용하신 것 같아요!!! 저는 아직 익숙하지 않아서 많은 요소가 따로 설정되어있는데 배워갑니다😃
버튼 색이 아주 눈에 잘 들어오네요🤩
술먹기가 취미.. 역시 전교회장~~~
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.
현재 세부적인 컴포넌트까지 포함해서 모든 페이지 컴포넌트가 /pages 폴더 안에 있네요~
p2) 컴포넌트와 페이지를 분리하면 더 좋은 구조가 될 것 같아요!!
/pages: 페이지 단위 컴포넌트(Login, SignUp, MyPage 등)
/components: 재사용 가능한 작은 컴포넌트(버튼, 입력창, Header 등, 페이지 내 요소 등)
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.
p2) api 요청과 관련된 함수도 api 관련 파일끼리 묶어두는것도 좋을 것 같아요.
baseURL: import.meta.env.VITE_APP_BASE_URL, | ||
}); | ||
|
||
export const checkingInstance = axios.create({ |
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.
axios 인스턴스가 instance와 checkingInstance로 분리되어 있는데, 혹시 이렇게 분리한 이유가 있으신가요??
p1) 토큰이 필요 없는 요청에도 불필요한 헤더를 추가할 수 있으니, Axios 인스턴스를 하나로 통합하고 요청마다 조건적으로 헤더를 추가할 수 있게 하면 좋을 것 같아요 :)
const token = await ApiLogin(); | ||
localStorage.setItem('user', token); | ||
navigate('/mypage'); | ||
} catch (e) { |
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.
p2) 에러 code에 따라서 에러 메시지를 제공하는 것도 좋을 것 같습니다.
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.
4주차 과제하느라 고생많으셨습니다~!
👍👍👍
<> | ||
<RouterProvider router={router}/> | ||
</> |
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.
<> | |
<RouterProvider router={router}/> | |
</> | |
<RouterProvider router={router}/> |
요기는 굳이 <></> 로 감싸주지 않아도 될 것 같고, prettier 설정이 제대로 안되어 있는 것으로 보이네요..!
import { Theme } from '../styles/Themes'; | ||
|
||
const Mypage = () => { | ||
const [viewMode, setViewMode] = useState('hobby'); |
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 [viewMode, setViewMode] = useState('hobby'); | |
const [viewMode, setViewMode] = useState<'hobby' | 'info'>('hobby'); |
viewMode의 경우 'hobby'또는 'info'값을 가지므로, 위와 같이 타입을 지정해주면 조금 더 안전한 코드를 짤 수 있을 것 같습니다.
const [selectedHobby, setSelectedHobby] = useState(''); | ||
|
||
useEffect(() => { | ||
setSelectedHobby(`${no}번 사용자의 취미: ${othersHobby}`); |
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.
selectedHobby에다가 화면에 그려진 텍스트를 그대로 다 저장하기 보다는,
텍스트는 화면을 그리는 부분에서 추가해주는 것이 더 좋을 것 같습니다!
이렇게 저장하게 되면 no, othersHobby 같은 데이터 값과 UI텍스트가 결합되어, 상태관리가 복잡해지는 문제가 있을 수 있습니다.
데이터와 UI를 분리해서 관리하는 것이 유지보수성과 확장성을 높여줍니다!
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.
지금보니 다시 검색을 하는 과정에서 검색된 no값이 바뀌어버리는 문제 때문에 이렇게 구현을 하신 것 같은데, 이 경우에도
text를 통째로 state에 저장하는 것이 아니라, 검색을 할 때마다, 현재 검색한 no를 새로운 state에 저장하게 되면 해결할 수 있을 것 같습니다!
또한 다른사람의 취미를 조회하는 함수에서 이 로직을 넣어준다면 useEffect도 굳이 사용할 필요가 없을 것 같습니다!
const response = await checkingInstance.get('/user/my-hobby'); | ||
return response.data.result.hobby; | ||
} catch (e) { | ||
console.log(e); |
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.
console.log(e); | |
console.error(e); |
에러메시지의 경우 console.log보다는 console.error로 찍어주면 더 좋을 것 같아요~
const fetch = async () => { | ||
const hobby = await getMyHobby(); | ||
setHobby(hobby); | ||
}; | ||
fetch(); |
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.
혹시 여기 부분에 useEffect 내에서 함수를 선언하고 사용하신 이유가 있으신가요?
어차피 안에서만 선언되고 사용한다면 굳이 함수를 선언하지 않아도 될 것 같고, 함수의 재사용을 고려한다면 useEffect 밖에 선언되는 것이 더 좋지 않을까요?
value={no || ''} | ||
onChange={(e) => setNo(Number(e.target.value))} | ||
/> | ||
<Button onClick={() => getOtherHobby()}>검색</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.
<Button onClick={() => getOtherHobby()}>검색</Button> | |
<Button onClick={getOtherHobby}>검색</Button> |
요렇게 작성해도 괜찮지 않을까요?
const clickFixButton = async () => { | ||
if (!password && !hobby) { | ||
alert('비밀번호 혹은 취미를 입력해주세요'); | ||
return; | ||
} | ||
try { | ||
await checkingInstance.put('/user', myInfo); | ||
alert('정보 수정에 성공했습니다.'); | ||
navigate('/'); | ||
} catch (error) { | ||
console.log(error); | ||
} | ||
}; | ||
|
||
useEffect(() => { | ||
if (password && hobby) { | ||
setMyInfo({ | ||
hobby, | ||
password, | ||
}); | ||
} else if (password && !hobby) { | ||
setMyInfo({ password }); | ||
} else { | ||
setMyInfo({ hobby }); | ||
} | ||
}, [password, hobby]); |
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 clickFixButton = async () => { | |
if (!password && !hobby) { | |
alert('비밀번호 혹은 취미를 입력해주세요'); | |
return; | |
} | |
try { | |
await checkingInstance.put('/user', myInfo); | |
alert('정보 수정에 성공했습니다.'); | |
navigate('/'); | |
} catch (error) { | |
console.log(error); | |
} | |
}; | |
useEffect(() => { | |
if (password && hobby) { | |
setMyInfo({ | |
hobby, | |
password, | |
}); | |
} else if (password && !hobby) { | |
setMyInfo({ password }); | |
} else { | |
setMyInfo({ hobby }); | |
} | |
}, [password, hobby]); | |
const clickFixButton = async () => { | |
if (!password && !hobby) { | |
alert('비밀번호 혹은 취미를 입력해주세요'); | |
return; | |
} | |
try { | |
await checkingInstance.put('/user', { | |
hobby, | |
password, | |
}); | |
alert('정보 수정에 성공했습니다.'); | |
navigate('/'); | |
} catch (error) { | |
console.log(error); | |
} | |
}; | |
useEffect까지 사용하며 myInfo라는 state를 별도로 관리할 필요없이 api 요청을 보낼때 위와같이 보내는 게 더 좋을 것 같습니다! (물론 hobby나 password가 빈칸이라면 객체에서 빼는 처리는 해줘야합니다.)
이렇게 하는게 더 좋은 이유는
- 불필요한 useEffect 사용
- 같은 값을 여러 state에서 관리하면서 생길 수 있는 데이터 동기화 문제
여기서 hobby와 password라는 각 state에 데이터가 있는데, 이를 myInfo라는 별도의 state에도 저장하게 되면 똑같은 hobby와 password가 여러개의 state에 저장되게 됩니다.
이렇게 되면 데이터가 제대로 동기화 되지 않을 수도 있고, 데이터의 흐름을 파악하기 어려워져 유지보수나 가독성 측면에서도 좋지 않습니다!
const response = await instance.post('/user', { | ||
username: formData.id, | ||
password: formData.password, | ||
hobby: formData.hobby, | ||
}); |
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.
interface FormData {
username: string; // 기존 id -> username으로 수정
password: string;
hobby: string;
}
와 같이 변수 명을 API 요청 변수명과 맞춘다면
const response = await instance.post('/user', { | |
username: formData.id, | |
password: formData.password, | |
hobby: formData.hobby, | |
}); | |
const response = await instance.post('/user', { ...formData }); |
과 같이 훨씬 간결하고 깔끔하게 작성할 수 있을 것 같습니다!
interface FormData { | ||
id: string; | ||
password: string; | ||
hobby: string; | ||
} |
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.
FormData 타입이 여러파일에서 동일하게 사용되고 있는데, 이런경우 공통 type으로 빼고, export/import해서 가져다 쓰는 것이 더 좋을것 같습니다!
<Button disabled={value.length == 0 || value.length > 8} onClick={onNext}> | ||
다음 | ||
</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.
Input이나 Button등은 공통 컴포넌트로 분리하는 방식도 고려해보면 좋을 것 같습니다~
✨ 구현 기능 명세
💡 기본 과제
취미
,내 정보
메뉴 탭로그아웃
버튼취미
,내 정보
취미 페이지, 내 정보 페이지 출력 (1개의 페이지로 구현해도 되고, url 달라도 됨)🔥 심화 과제
공유과제
제목:
링크 첨부 :
❗️ 내가 새로 알게 된 점
❓ 구현 과정에서의 어려웠던/고민했던 부분
🥲 소요 시간
18h
🖼️ 구현 결과물
https://youtu.be/XXQkbN9q6mg