-
Notifications
You must be signed in to change notification settings - Fork 44
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
[이주안] Week13 #457
The head ref may contain hidden characters: "part3-\uC774\uC8FC\uC548-week13"
[이주안] Week13 #457
Conversation
[Fix] delete merged branch github action
…y-Mission into part2-이주안-week8
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.
잘 작성해주셨습니다 👍
기본적으로 코드 가독성도 좋고, 스타일명만으로도 UI 구조가 읽혀서 좋은 것 같습니다.
고생하셨습니다!
</div> | ||
</div> | ||
); | ||
}; |
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.
잘 작성해주셨습니다! 👍
</div> | ||
</div> | ||
); | ||
}; |
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.
classnames 가독성이 있고 해당 스타일명만으로도 코드 구조가 읽혀서 잘 작성해주신 것 같습니다
</DeleteModal> | ||
)} | ||
|
||
{addemodal && ( |
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.
addemodal 이라는 이름보다는 show라는 prefix가 붙은 이름이 더 좋을 것 같습니다
</div> | ||
)} | ||
|
||
{deleteModal && ( |
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.
deleteModal 이라는 이름보다는 show라는 prefix가 붙은 이름이 더 좋을 것 같습니다
)} | ||
</div> | ||
); | ||
}; |
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.
뭔가 보여줘야하는 조건의 state값의 경우 prefix를 show를 붙여주시면 좋을 것 같습니다.
)} | ||
</div> | ||
); | ||
}; |
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.
만약 각각 형태의 모달을 보여줘하신다면, state를 하나로 하는 방법이 좋을 것 같습니다.
그렇다면 boolean값보다는 showModalType이라고 해서 delete, add, share string타입을 set하여
타입별로 해당 타입의 모달을 보여주면 좋을 것 같습니다
const router = useRouter(); | ||
const [value, setValue] = useState<string>(initialValue); | ||
const [search, setSearch] = useState<string>(''); | ||
const { q } = router.query; |
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.
q 보다는 조금 더 자세한 이름이 어떤 query값인지 알 수 있어서 그렇게 사용해주시면 좋을 것 같습니다
|
||
const cx = classNames.bind(styles); | ||
|
||
export const NavigationBar = ({ name, profile, email }: User) => { |
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로 해주신 부분을 잘 해주신 것 같습니다
export const getFolders = async () => { | ||
const response = await axios.get(`/users/1/folders`); | ||
return response.data.data; | ||
}; |
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.
api등을 utils로 분리하신 부분은 관리측면에서 좋아서 잘 작성해주셨습니다
const user = await getUser(); | ||
const link = await getLinks(); | ||
const folder = await getFolders(); | ||
|
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.
await으로 순차적으로 받을 경우 api 응답 속도가 지연될 수록 느리게 데이터들이 set될 수 있기에, 해당 부분을 추후에는 Promise.all을 사용해주시면 좋습니다. Promise.all 에 대해서 확인해보시면 좋을 것 같습니다
요구사항
기본
12주
기본 요구사항
심화 요구사항
13주
주요 변경사항
스크린샷
멘토에게