[2단계 - 자주 가는 음식점] 엽토(김건엽) 미션 제출합니다. #71
Conversation
| } | ||
| } | ||
|
|
||
| export default Modal; |
There was a problem hiding this comment.
2단계에서 모달이 재사용되는지를 모르고 1단계에서 재사용할 수 있도록 만들지 않았어요.. 1단계에서 큰 섹션단위로 나누면 그게 컴포넌트라고 생각했는데 2단계 미션을 하면서 그게 아니라는 걸 깨닫게 되었습니다. 물론 RestaurantsList는 자주가는 음식점 목록을 렌더링할때 재사용할 수 있었습니다. Modal이란 추상 컴포넌트를 만들고 상속해서 AddModal, InfoModal을 만들어 주었는데요.. Modal이란 추상 클래스를 잘 추상화한진 모르겠습니다.. 왜냐하면 나누어줬을 뿐인데 원래대로 작동이 잘 안되어서 삽질을 많이 했거든요.. 너무 괴로웠습니다..
There was a problem hiding this comment.
ㅠㅠㅠ 엽토님 코멘트만 봐도 고통이 느껴집니다.
이런 현상은 사실 빈번하게 일어나는 일인데요 기획이 바뀌거나 자연스럽게 확장하면서 재사용되는지 모르고 만들었다가, 공통 모듈로 분리하는 경우도 있고 반대의 경우도 생깁니다.
Modal 의 클래스는 잘 만들어주신거 같은데 AddModal 과 InfoModal 의 역할이 정확히 파악이 안되서 어떤 역할로 만들어 주셨는지 적어주시면 좋을거 같아요~
There was a problem hiding this comment.
add 모달은 음식점 추가해주는 모달이고, Info 모달은 음식점 정보를 보여주는 모달입니다!
| this.$target = $target; | ||
| this.sortedCallback = restaurants.getSelectedRestaurantsList.bind(restaurants); | ||
| this.getFavoriteCallback = restaurants.getFavoriteRestaurantList.bind(restaurants); | ||
| this.infoRenderCallback = infoModal.render.bind(infoModal); | ||
| this.restaurantItem = new RestaurantItem(); | ||
| this.setState(restaurants.restaurantsList); | ||
| } | ||
|
|
There was a problem hiding this comment.
RestaurantList 컴포넌트는 항상 상태가 변경되면 렌더링 되어야하는 컴포넌트라 생각해요.. 이 컴포넌트에선 즐겨찾기 클릭 이벤트도 있고, 리스트 클릭했을 때 모달을 띄우는 이벤트도 있고.. 그러다 보면 이 컴포넌트에서는 즐겨찾기를 했을 때 좋아하는 음식점으로 레스토랑 상태를 바꿔주는 도메인 로직도 필요하게되고, 모달을 렌더링하는 로직도 필요하고.. 이러다보니 너무 복잡해지더라구요.. 이런 부분이 제가 위에서 말씀드렸던 것의 예시입니다!
There was a problem hiding this comment.
흠,, 코드와 엽토의 코멘트를 보니 어떤 부분에서 고민을 많이 했는지 알 거 같습니다.
제가 드리고 싶은 코멘트는 이런 구조에서는 복잡해 질 수 밖에 없는 구조라고 생각합니다.
여기서 덜 복잡해 지기 위해 음식점 리스트와 자주가는 음식점 리스트를 분리해볼수 도있을거라 생각합니다.
그렇다면 해당 RestaurantList 는 Restaurant 로 변경되고 음식점 리스트와 자주가는 음식점 리스트의 렌더링 컨트롤러와 모달 컨트롤러 역할을 할 수 있을거 같네요!
구조적으로 방대해지거나 지금처럼 이상함을 느낄때에는 내가 생각한 구조의 이상이 있음을 인지하고 다른 방법으로 변경해보는것도 좋을거 같아요
There was a problem hiding this comment.
아! 음식점 리스트와 자주가는 음식점 리스트를 나눠야겠단 생각을 못했네요... Restaurant라는 추상 컴포넌트를 만들어서 List, FavoriteLIst 이렇게 각각 상속 시켜서 했으면 결국 Restaurant가 다 가지고 있으니 상속해준 각각의 컴포넌트들은 필드수가 줄어 들어서 코드가 좀 덜 복잡하지 않았을까 싶네요.. 즉, 캐빈 말씀대로 Restaurant이 컨트롤러 역할을 할 수 있는거죠!(아닌가요..? 하하)
| favorites: false, | ||
| }; | ||
|
|
||
| export { yeoptoRestaurant, doriRestaurant, gongwonRestaurant }; |
There was a problem hiding this comment.
이번엔 Mock Data를 넣어주었습니다..! 바뀌는 상태가 있어서 freeze하지 않았습니다!
| window.addEventListener('beforeunload', () => { | ||
| localStorage.setItem('restaurants', JSON.stringify(restaurants.restaurantsList)); | ||
| }); | ||
|
|
There was a problem hiding this comment.
이 부분이 로컬스토리지와 관련해 변경된 로직인데요. 예를 들어 즐겨찾기 버튼을 클릭할 때마다 렌더링 되게 코드를 짰는데 매번 클릭 시에 로컬스토리지 값을 갱신해주는건 너무 비효율적이라 생각했어요.. 사용자 입장에서 상태가 변경될때마다 렌더링되기 때문에 변하는게 보일테고 새로고침할 때 값빠지는 것만 고려한다면 새로고침 될 시에만 로컬스토리지 값을 갱신해주면 되지 않을까란 생각에 이렇게 변경하였습니다!
흠,, 이 코멘트를 듣고 제가 잘 이해한건지는 모르겠으나, 말이 조금 안맞는거 같아서 다시 정리하면
제가 이해한게 틀리다면 다시 코멘트 부탁드립니다 ㅠㅠ |
There was a problem hiding this comment.
안녕하세요 엽토~
미션수행하느라 고생많으셨습니다 ㅠㅠ 고통을 많이 받았다고 하셨는데 그만큼 고민도 많이했고 배운것도 많았다고 생각합니다! 💯
전체적인 코드 평은 각 파일의 역할이 분명하지 않고, 전체적으로 코드 하나하나를 보면 괜찮은데 조합하는 로직이나 컨트롤러 과정이나 서로의 관심사가 분리되어 있지 않아서 이렇게 복잡해지고 유지보수나 기능확장이 어렵다고 생각합니다 ㅠㅠㅠ
그래서 주어진 기획서랑 피그마를 보면서 대략적으로 전체적인 구조를 한번 잡아보시는걸 추천드려요!
예를 들어서 이부분은 어떤 폴더의 어떤 파일에서 보여줄지를 도식화 하시면 좋을거같아요!
추가로 지금 엽토가 한 것도 같이 도식화해서 비교해보면 더 좋겠네요 🙂
그렇게 하다 보면 어떤 부분때문에 복잡해졌는지 파악하실수 있을거 같아요!
궁금한부분은 코멘트로 남겨드렸으니, 제가 남겨드린 부분 고민해보시고 다시 코멘트로 남겨서 같이 얘기해보면 좋을거같아요 :)
|
|
||
| .favorite-icon { | ||
| cursor: pointer; | ||
| } |
There was a problem hiding this comment.
혹시 이 css 파일은 왜 스네이크 형식으로 파일명 되어있는지 알 수 있을까요??
There was a problem hiding this comment.
css파일명도 js파일과 통일되게 카멜케이스로 만들어줘야하는건가요?!
src/css/tabbar.css
Outdated
| font-weight: 700; | ||
| width: 50%; | ||
| color: var(--grey-300); | ||
| padding: 15px 15px; |
There was a problem hiding this comment.
| padding: 15px 15px; | |
| padding: 15px; |
같으면 그냥 만 써줘도 될거 같아요~
| window.addEventListener('beforeunload', () => { | ||
| localStorage.setItem('restaurants', JSON.stringify(restaurants.restaurantsList)); | ||
| }); | ||
|
|
src/components/RestaurantFilter.js
Outdated
| closeFilter() { | ||
| return $('.restaurant-filter-container').classList.remove('restaurant-filter-open'); | ||
| } |
src/components/RestaurantFilter.js
Outdated
| openFilter() { | ||
| return $('.restaurant-filter-container').classList.add('restaurant-filter-open'); | ||
| } |
src/components/RestaurantItem.js
Outdated
| <h3 class="restaurant__name text-subtitle">${restaurant.name}</h3> | ||
| <img id="${restaurant.id}" class="favorite-icon" src="./${ | ||
| restaurant.favorites ? 'favorite-icon-filled' : 'favorite-icon-lined' | ||
| }.png" alt="${restaurant.id}"> |
There was a problem hiding this comment.
alt 태그를 id 값 말고 다른 값으로 활용해보면 어떨까요?? 추가로 alt 값이 어떤 역할을 하는지도 살펴보면 좋을거 같네요~ 👍
| remove(id: string) { | ||
| this.#restaurantsList = this.#restaurantsList.filter(restaurant => restaurant.id !== id); | ||
| } |
| cy.get('.gnb__button').click(); | ||
| cy.get('#category').select('한식'); | ||
| cy.get('#name').type('공원네 밥집'); | ||
| cy.get('#distance').select('10분 내'); | ||
| cy.get('#description').type('집 앞 분식점'); | ||
| cy.get('form .button--primary').click(); |
There was a problem hiding this comment.
이 형식이 계속해서 반복으로 나타나는데 공통함수로 빼보는건 어떨까요?
| } | ||
| } | ||
|
|
||
| export default Modal; |
There was a problem hiding this comment.
ㅠㅠㅠ 엽토님 코멘트만 봐도 고통이 느껴집니다.
이런 현상은 사실 빈번하게 일어나는 일인데요 기획이 바뀌거나 자연스럽게 확장하면서 재사용되는지 모르고 만들었다가, 공통 모듈로 분리하는 경우도 있고 반대의 경우도 생깁니다.
Modal 의 클래스는 잘 만들어주신거 같은데 AddModal 과 InfoModal 의 역할이 정확히 파악이 안되서 어떤 역할로 만들어 주셨는지 적어주시면 좋을거 같아요~
|
케빈!! 리뷰 감사합니다!! 로직을 수정하고 있는데 시간이 모자라서 부랴부랴 커밋하느라 한꺼번에 커밋했습니다.. 아직 끝마치지 못했습니다.. 리뷰요청 일단 보내놓고 오늘까지 마무리하려고 합니다..! |
|
케빈! 리팩터링을 했고,
여기서 말했던 버그도 고쳤습니다~ 처음에 RestaurantItem 클래스엔 템플릿, 템플릿을 합쳐주는 메서드 두개만 있었어요! RestaurantsList에서 합쳐주는 메서드를 호출해서 합쳐진걸 렌더링 하도록 했더니 이벤트가 합쳐진 곳 하나에만 붙었던 것 같더라구요! 그래서 재 클릭시 이벤트가 빠져있었던 것 같습니다.. 그래서 이것 또한 아이템 하나씩 렌더링되게 해주었습니다. 렌더링할 때마다 이벤트도 붙여주었구요. 이번 미션을 하다보니 정적으로 한번만 렌더링 되는 것은 이벤트 한번만 붙여주면되지만, 동적으로 렌더링되는 것은 렌더링시에 꼭 이벤트를 붙여야 재렌더링시 이벤트가 잘 붙는걸 알게되었습니다.. 코드는 리펙터링 전과 크게 다를 것 없지만, 각 컴포넌트에서 필요한 이벤트 함수들은 컴포넌트 생성시에 주입해줘서 각 컴포넌트에서 알아서 하도록 만들었어요! 그랬더니 이 컴포넌트에선 어떤 이벤트 함수들이있는지 잘 알 수 있어서 흐름이 좀 보이는 것 같습니다.(물론 저만 그럴수도 있습니다.. 하하..)
이 부분도 클래스와 컴포넌트는 별개의 문제인데 머리가 복잡하다보니 꼬아서 생각한 것 같습니다! 이 부분도 명확히 이해할 수 있었습니다! 감사합니다! 이번 미션 리뷰 정말 감사합니다 케빈! |
아닙니다 저도 이전보다 훨씬 코드 파악이 좋았던거 같아요 👍 고생많으셨습니다 엽토! |
There was a problem hiding this comment.
안녕하세요 엽토~
구조 때문에 많이 고생하신거 같은데, 이번 미션을 통해 많이 배우고 경험했으면 좋겠습니다.
지금 단계에서 이러한 고민들이 생기는건 자연스러운 일이라고 생각합니다! 그러니 좌절 마시공 이것저것 여러가지 경험해보면서 각 미션의 취지(이번 미션은 타입스크립트, DOM, HTML) 에 맞추어 하나씩 공부해나가면 좋을거 같아요!
추가적으로 말씀드리고 싶은건 앞으로 개발을 시작하실때, 먼저 코드를 짜시기 보단, 큰틀에서 구조를 잡고 대략적인 코드를 짜고 개발하시는걸 추천드려요!
궁금한부분은 코멘트로 남겨드렸으니, 제가 남겨드린 부분 고민해보시고 다시 코멘트로 남겨서 같이 얘기해보면 좋을거같아요 :)
src/components/modal/InfoModal.js
Outdated
| const removeButton = $('.button--secondary'); | ||
|
|
||
| removeButton.addEventListener('click', () => { | ||
| this.removeRestaurant($('.favorite-icon').alt); |
There was a problem hiding this comment.
삭제하는 그 레스토랑을 알려면 alt에 id를 넣어줘서 alt값으로 가져와야겠다고만 생각했던 것 같습니다.. 바보같았네요.. 그냥 id로 가져오면 되는 건데.. alt는 name으로 바꾸어주었고, alt대신 .id를 사용해서 삭제시켜주도록 했습니다.
| if ($('.current').innerText === '모든 음식점') { | ||
| this.toggleModalOpen(); | ||
| listRender(); | ||
| return; | ||
| } | ||
|
|
||
| this.toggleModalOpen(); | ||
| favoriteRender(); |
There was a problem hiding this comment.
setRemoveEvent 가 너무 많은 일까지 하는거 같아요~
함수를 분리해도 좋고 네이밍을 바꿔봐도 좋을거 같아요~
There was a problem hiding this comment.
Remove만 하는게 아니라 렌더링까지 해주는 이벤트다보니 setRemoveHandleEvent로 변경했습니다~
src/components/RestaurantsList.js
Outdated
| setItemEvent(restaurantList) { | ||
| restaurantList.forEach(restaurant => { | ||
| $(`#${restaurant.id}`).addEventListener('click', e => { | ||
| if (e.target.id === '') { | ||
| this.infoRenderCallback(restaurant, this.render.bind(this), this.renderFavoriteRestaurant.bind(this)); | ||
| } | ||
| }); | ||
| }); |
There was a problem hiding this comment.
이 로직은 RestaurantItem에 있어도 되는 로직 아닐까요?
There was a problem hiding this comment.
아 이건 Outdated입니다! 6시 이전에 다 못했을때 RestaurnatList에 있었습니다! 지금은 RestaurantItem에 있어요!
| this.$target = $target; | ||
| this.sortedCallback = restaurants.getSelectedRestaurantsList.bind(restaurants); | ||
| this.getFavoriteCallback = restaurants.getFavoriteRestaurantList.bind(restaurants); | ||
| this.infoRenderCallback = infoModal.render.bind(infoModal); | ||
| this.restaurantItem = new RestaurantItem(); | ||
| this.setState(restaurants.restaurantsList); | ||
| } | ||
|
|
There was a problem hiding this comment.
흠,, 코드와 엽토의 코멘트를 보니 어떤 부분에서 고민을 많이 했는지 알 거 같습니다.
제가 드리고 싶은 코멘트는 이런 구조에서는 복잡해 질 수 밖에 없는 구조라고 생각합니다.
여기서 덜 복잡해 지기 위해 음식점 리스트와 자주가는 음식점 리스트를 분리해볼수 도있을거라 생각합니다.
그렇다면 해당 RestaurantList 는 Restaurant 로 변경되고 음식점 리스트와 자주가는 음식점 리스트의 렌더링 컨트롤러와 모달 컨트롤러 역할을 할 수 있을거 같네요!
구조적으로 방대해지거나 지금처럼 이상함을 느낄때에는 내가 생각한 구조의 이상이 있음을 인지하고 다른 방법으로 변경해보는것도 좋을거 같아요
|
케빈이 제안해준 변경 사항들을 고민해보고 적용해보았습니다~
이 부분에서 깨달은게 있었어요~ 있는걸 재사용할 생각만 했지 나눌 생각은 못했네요! 이 부분에 대해서 코멘트를 남겨놨습니다! 따로 분리해보고 싶었는데 다음 미션이 촉박해서요..! 영화리뷰 미션에서 이런 상황이 온다면 적용해보도록 하겠습니다!(아마 2단계에서 적용하지않을까란 생각이..)
이 습관을 고치기 정말 어렵더라구요.. 뭔가 설계를 한답시고 했는데 짜다보면 잘못된 설계라 갈아엎으려다 보니 코드가 복잡해지는 현상이 반복되는 것 같아요ㅠ 팁 정말 감사합니다! 이번 미션도 느끼고 배운게 많았습니다! DM으로도 말씀드렸지만 다시한번 정말 감사합니다 케빈! |
안녕하세요 케빈! 괴로운 엽토입니다..
이번 2단계 미션은 정말x100 괴로웠습니다..
1단계 처음 리뷰요청을 했을 때처럼 굉장히 복잡한 코드가 되었습니다. 2단계에 동적으로 렌더링하는 곳에서 이벤트들이 많아지니까 이벤트 핸들링하기 어렵더라구요.. 1단계에 보신 것 처럼 전역에서 이벤트를 관리해주면서 이벤트에 필요한 로직은 바인딩해서 넘겨줬었는데, 동적 렌더링 시에 replaceChilderen 했다가 다시 만들어서 넣어주다보니 이벤트 함수를 전역에서 관리 못하고 렌더링시에 계속 붙여줘야 했습니다. 그러다보니 컴포넌트간에 의존이 너무 심해지고 코드가 복잡해진 것 같아요. 그럼 도대체 어떻게 구조를 잡아야 하는가에 대한 고민을 많이 해봤지만 제 역량이 부족하여 생각나지가 않았습니다.. 새로운걸 찾고 학습해서 다 뜯어고치기엔 시간이 너무 부족했습니다. 최선을 다 했는데 돌아가는 쓰레기도 우여곡절 끝에 만들어서 정말 괴로웠던 것 같습니다..😰
이렇게 복잡해지는 코드를 보면서 문득 의문점이 생겼습니다. 과연 컴포넌트를 만들 때 클래스를 사용하는게 맞을까란 의문입니다. 만약 A 컴포넌트에서 이벤트가 발생하였을 때 B 컴포넌트를 그려줘야한다면 A컴포넌트에 B컴포넌트간에 의존성이 생긴다고 저는 생각하는데요. 클래스 즉, 객체끼리는 서로에 대해 모르고 간섭하지 않아야한다고 알고있는데 위 예시는 A 컴포넌트와 B 컴포넌트가 서로에 대해 알 수 밖에 없지 않나요..? 알고있는 거라면 서로가 모르도록 구현할 수 있는 방법이 있을까요? 아님 당연히 이벤트로 엮인 컴포넌트들은 서로가 알 수 밖에 없는 건가요? 아직 제가 의존한다라는 의미를 아직 정확하게 모르는 것 같습니다.. 컴포넌트를 클래스로 만들다보니 이렇게 서로가 서로를 알고 복잡해지는게 맞나라는 생각과 이렇게 아니면 그럼 어떻게 구현해야하지라는 두가지 생각이 계속 들더라구요.. 함수로 구현하자니 상태관리가 너무 어려울 것 같고.. 정답은 없겠지만 어떻게 하면 바닐라 JS로 깔끔하게 짤 수 있을까요..
어찌저찌 돌아가게는 만들었어요.. 하지만 딱 한가지의 버그가 있습니다.. 음식점 목록에서 음식점 하나를 클릭하고 닫기를 누른 뒤 클릭했던 음식점을 다시 클릭하면 이벤트가 먹질않더라구요.. 다른 음식점을 클릭해서 이벤트 발생 시키고 닫기했을 때 그 전에 클릭했던 음식점이 클릭 되더라구요.. 제가 할 수 있는 모든 방법을 다 시도해봤는데 결국 안되어서 포..기했습니다.. 괴롭네요..🤯
1단계랑 로컬스토리지 관련해서 로직이 좀 변경된게 있습니다! 그래서 배포링크를 들어가실 때 인터넷 사용 기록을 삭제하고 들어가주시면 정말 감사하겠습니다..!
나머지 내용은 코멘트로 남기겠습니다!
복잡한 코드 읽어주셔서 감사합니다! 이번에도 잘.. 부탁드립니다 케빈..😭