[점심 뭐 먹지 미션 Step 2] 우디(류정우) 미션 제출합니다. #85
Conversation
There was a problem hiding this comment.
안녕하세요 우디! 금방 미션 진행하고 오셨군요 👍
컴포넌트에 남길 로직과 커스텀 훅으로 뺄 로직을 분리하는건 저도 아직까지 어려운 것 같아요 🥲
점차 여러 미션을 통해서 어떻게 분리하는게 좋을지 우디만의 방식을 만들어가보면 좋을 것 같습니다!
훅은 비즈니스 로직, 즉 컴포넌트의 상태나 나아가서는 상태관리 라이브러리의 상태들을 관리하는 로직을 모아두는 specific 한 훅이 있을 수 있고, 우디의 코드에서처럼 로컬스토리지를 다루는 것과 같은 범용적인 훅이 있겠죠!
모든걸 범용적으로 만들 필요는 없고 용도에 따라 네이밍을 잘 해둬서 팀원과 헷갈리지 않도록 분리해두면 좋을 것 같아요!
portal 의 경우에는, 우디가 지금 modal 을 app 바로 하위에 생성해서 잘 못 느낄 수도 있지만! 예를 들어 RestaurantList 에 생성했다면 dom tree 가 꼬였을거예요. body 하위에 생성하지 못해 z index 도 꼬이게 될테니까요! portal 을 사용하면 어디에 dom element 가 생성되는지, 어떻게 컨트롤이 되는건지 등등을 공부해보시면 될거예요~!
딱히 큰 리뷰는 없고 코멘트만 살펴봐주세요! 그리고 useCallback 이나 useMemo 같은 훅도 알아보시면 도움될 것 같습니다~!
고생 많으셨습니다 🙇🏼♀️
| this.setState({ | ||
| const openModal = (id: Restaurant['id']) => { | ||
| setModalState({ | ||
| ...modalState, |
There was a problem hiding this comment.
setState 의 파라미터를 함수로 정의할 수도 있습니다!
setState((prev) => ({ ...prev, ... }));There was a problem hiding this comment.
헤인트의 예시처럼 이전 상태를 참조하고 반환하는 방식이 더 안정적이겠군요!
제가 쓴 코드는 객체를 병합하고 있기 때문에, 이전 상태와 충돌할 가능성이 있겠네요
감사합니다🤩
| setModalState({ | ||
| ...modalState, | ||
| isModalOpen: false, |
There was a problem hiding this comment.
setModalState 파라미터로 함수를 넘길 수도 있습니다!
setState((prev) => ({ ...prev, ... });| import { CATEGORIES, SORTING_TYPES } from '../../constants'; | ||
|
|
||
| interface Props { | ||
| setCategory: (category: Category | '전체') => void; |
There was a problem hiding this comment.
props 를 넘겨주는 사용처(=App) 과 props 를 넘겨받는 구현처(=Filter)를 나눠서 생각해보면 좋을 것 같아요!
Filter 에서 카테고리가 바꼈을 때 사용처에서 할 일을 알아야 할까요? 카테고리가 바꼈을 때 실행할 함수를 props 로 받기만 하면 되지 않을까요?!
onChangeCategory(event: ChangeEvent<HTMLSelectElement>): void;같이 사용처에서 카테고리를 저장한다 와 같은 비즈니스 로직을 알 필요없이 핸들러만 제공해주는건 어떤가 해서요!
There was a problem hiding this comment.
여러 곳에서 Filter 를 사용하면 모든 곳에서 카테고리가 바꼈을 때 카테고리를 저장한다는 보장은 없죠? 여러가지 일을 할 것이기 때문에 props 네이밍은 범용적이고 명확하면 좋을 것 같아요!
| ); | ||
| }; | ||
|
|
||
| export default RestaurantDetail; |
There was a problem hiding this comment.
https://developer.mozilla.org/en-US/docs/Web/Accessibility/ARIA/Attributes/aria-modal
모달 관련된 접근성도 한번 훑어보시면 좋을 것 같아요~!
| margin: 0; | ||
| ${textSubtitle} |
There was a problem hiding this comment.
아래 스타일들도 마찬가지지만, 이런식으로 하면 만약 textSubtitle 내에 margin 값이 있으면 margin: 0; 은 오버라이딩 될거예요!
textSubtitle 을 커스텀 스타일보다 위에 위치시키는게 좋지 않을까요?
| const onClickRestaurantList = ({ target }: React.MouseEvent) => { | ||
| const { id } = (target as HTMLUListElement).closest('li'); | ||
| props.openModal(id); | ||
| }; |
There was a problem hiding this comment.
| const onClickRestaurantList = ({ target }: React.MouseEvent) => { | |
| const { id } = (target as HTMLUListElement).closest('li'); | |
| props.openModal(id); | |
| }; | |
| const onClickRestaurantList = ({ target }: React.MouseEvent<HTMLUListElement>) => { | |
| const { id } = target.closest('li'); | |
| props.openModal(id); | |
| }; |
| } | ||
| }, []); | ||
|
|
||
| return JSON.parse(localStorage.getItem(key) || '[]'); |
There was a problem hiding this comment.
음 코드만 봤을 때는 로컬스토리지에 데이터가 없는 경우
- 빈배열 리턴
- useEffect 에서 로컬스토리지에 데이터 채움
setState 같은 함수를 호출하지 않았으므로 리렌더링이 되지 않아 state 가 다시 변하기 전까진 빈 배열이 계속 사용될 것 같은데요! 그렇진 않았나요?
헤인티 안녕하세요!!!
컴포넌트는 렌더링과 이벤트 로직만을 가지도록 �다른 로직들을 숨기자!step2에서 가장 신경썼던 부분입니다
하지만 아직까지 어디까지가 렌더링을 위한 로직인지, 어디까지가 이벤트 처리 로직인지 그 기준이 어려웠어요
상태를 변경하는 로직은 리렌더링을 위한 로직인 것 같은데, 상태도 도메인 로직과 함께 custom hook으로 숨기는 것이 더 깔끔한 것 같아요
조금 더 많이 분리해보면서 감을 익혀야 할 것 같습니다😭
custom hook
�custom hook은 범용성을 �가지도록 설계하는 것이 좋을까요?
범용성이 높은 hook들은 이미 대부분이 만들어져있으니 custom hook은 해당 프로젝트에 종속되도록 설계하는 것도 괜찮을 것 같다는 생각이 들어요
헤인티의 생각을 듣고 싶습니다!
portal
헤인티가 권했었던 portal에 대해서도 공부를 해봤어요
하지만 현재 미션에서 적용함으로서 얻을 수 있는 이점이 무엇인지는 끝내 생각해내지 못했어요ㅠㅠㅠ
ReactDom.createPortal을 작성한다 안한다말고 달라지는 코드가 없더라구요
protal에 대해서 조금 더 공부를 해본 후 적용해보고 싶습니다!
step2에서도 잘 부탁드리겠습니다🤩