-
Notifications
You must be signed in to change notification settings - Fork 46
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
[김한샘] week11 #360
The head ref may contain hidden characters: "part2-\uAE40\uD55C\uC0D8-week11"
[김한샘] week11 #360
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.
고생하셨습니다! 👍
- 코드 컨벤션 변경 내용이 diff로 들어와서 정작 중요한 로직 변경이 잘 안들어오네요,,, 다음부턴 컨벤션 변경은 안보이도록 해주세요!
- 코드를 관심사에 따라 더 나눠줄 여지가 있을 것 같아요. 좀 더 리팩토링 해보시면 좋을 것 같아요.
- 반복되는 코드가 많습니다. 좀 더 리팩토링 해보면 좋을 것 같습니다.
<button className="link-button">추가하기</button> | ||
</div> | ||
</form> | ||
const { openModal, setModalType } = useContext(ModalContext); |
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.
useContext(ModalContext);
이 코드가 반복되고 있네요 해결방법이 없을까요?
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.
이건 useToggle이랑 다른 점이 없는 것 같네요.
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.
Folder에 일반 JSX도 있고 Context도 있고 굉장히 많은 것들이 혼재해 있네요.
맥락에 따라 코드를 더 나눠보면 좋을 것 같습니다.
@@ -43,11 +48,16 @@ export const tabDataList = async () => { | |||
|
|||
export const userFoldersData = async () => { | |||
try { | |||
const response = await fetch(`${BASE_URL}api/users/1/links`); | |||
if (!response.ok) { | |||
const response = await axios.get(`${BASE_URL}api/users/1/links`); |
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.get(`${BASE_URL}api/users/1/links
코드가 반복되는 것 같습니다.
<li> | ||
<button> | ||
<img src={KakaoIco} alt="카카오톡 공유하기" /> | ||
<h3>카카오톡</h3> | ||
</button> | ||
</li> |
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.
이 부분이 반복되는 것 같습니다.
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.
modalType 판단과 더불어, 무엇을 그려야하는지 jsx가 그대로 노출 돼 있어 가독성과 유지보수성이 떨어집니다.
좀 더 리팩토링 해볼 여지가 있을 것 같습니다.
const cardTitleIcon = [ | ||
{ | ||
url: shareIcon, | ||
text: "공유", | ||
url: shareIcon, | ||
text: "공유", | ||
onClick: () => { | ||
setModalType("share"); | ||
openModal(); | ||
}, | ||
}, | ||
{ | ||
url: penIcon, | ||
text: "이름 변경", | ||
url: penIcon, | ||
text: "이름 변경", | ||
onClick: () => { | ||
setModalType("edit"); | ||
openModal(); | ||
}, | ||
}, | ||
{ | ||
url: deleteIcon, | ||
text: "삭제", | ||
url: deleteIcon, | ||
text: "삭제", | ||
onClick: () => { | ||
setModalType("folderDelete"); | ||
openModal(); | ||
}, | ||
}, | ||
]; | ||
]; |
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.
상수는 컴포넌트안에서 선언하지 않는 것이 좋습니다.
컴포넌트의 특성과 변수 생성 규칙을 생각해보면 그 이유를 알 수 있습니다.
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.
고생하셨습니다! 👍
- 코드 컨벤션 변경 내용이 diff로 들어와서 정작 중요한 로직 변경이 잘 안들어오네요,,, 다음부턴 컨벤션 변경은 안보이도록 해주세요!
- 코드를 관심사에 따라 더 나눠줄 여지가 있을 것 같아요. 좀 더 리팩토링 해보시면 좋을 것 같아요.
- 반복되는 코드가 많습니다. 좀 더 리팩토링 해보면 좋을 것 같습니다.
요구사항
기본
스크린샷
멘토에게