Skip to content
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

[14기 팝콘] Step2 상태 관리로 메뉴 관리하기 #284

Open
wants to merge 10 commits into
base: yeomgahui
Choose a base branch
from

Conversation

yeomgahui
Copy link

🎯 step2 요구사항 - 상태 관리로 메뉴 관리하기

  • [X ] localStorage에 데이터를 저장하여 새로고침해도 데이터가 남아있게 한다.
  • [ X] 에스프레소, 프라푸치노, 블렌디드, 티바나, 디저트 각각의 종류별로 메뉴판을 관리할 수 있게 만든다.
    • [ X] 페이지에 최초로 접근할 때는 에스프레소 메뉴가 먼저 보이게 한다.
  • [ X] 품절 상태인 경우를 보여줄 수 있게, 품절 버튼을 추가하고 sold-out class를 추가하여 상태를 변경한다.

Step 2 구현 완료하였습니다.
아쉬운 부분이 많습니다. 가장 먼저 코드가 너무 뒤죽박죽이네요.. 차차 수정하도록 하겠습니다..

  1. 상태관리를 위해서 파일을 나눠서 구현하고싶었는데, 여기서 너무 오래걸렸습니다. => 이부분은 민오님이나 유데미 강의를 보면서 확인해봐야겠네요..
  2. 카테고리 이벤트 리스너 추가하는 부분을 querySelectorAll을 이용해서 추가해주었는데, 다른 방법이 있는지 궁금하네요.

yeomgahui and others added 10 commits July 11, 2022 21:25
STEP 1 구현을 마쳤습니다.

개인적으로 구현하면서 아쉬웠던 부분을 생각해보면,

menu 추가시 수정 삭제를 위해 addEventListener를 등록 하는데, 메뉴 하나를 추가할때마다 모든 추가, 삭제 버튼의 eventListener를 다시 등록하도록 구현했습니다. => 개인적으로는 하나 추가할때마다 해당 수정, 삭제 버튼의 eventLIstener만을 추가하도록 하고 싶었는데 좋은 방법이 떠오르지 않았습니다.
JavaScript 파일을 기능 마다 분리하여서 작성하고 싶었는데, 어떻게 나눌지 모르곘어서 index.js 파일 내에 모든 로직을 다 작성했습니다. 이부분은 차차 다시 생각해보고 수정해보고 싶습니다.
Copy link

@dhrod0325 dhrod0325 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

안녕하세요 팝콘님! Step2 구현하느라 고생하셨어요!
리뷰 좀 남겨 보았어요~!
제 생각엔 중복되는 부분을 리팩토링 해보면 좋을 것 같아요! 😀


const submitMenu = (event) => {
event.preventDefault();
const keyword = $('#espresso-menu-name').value.trim();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$('#espresso-menu-name') 선언이 중복되고 있는데 윗부분에 한번만 선언해도 되지 않을까요~? 😀


const $menuName = $targetList.querySelector(".menu-name");
let updatedMenu = prompt('메뉴명을 수정하세요.', $menuName.innerText) ?? '';
updatedMenu = updatedMenu.trim().length > 0 ? updatedMenu : $menuName.innerText;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
updatedMenu = updatedMenu.trim().length > 0 ? updatedMenu : $menuName.innerText;
if(updatedMenu.trim().length === 0){
return;
}

이렇게 변경해도 같은 동작을 하지 않을까요~? 지금은 변경되지 않아도 아래 로직이 동작하면서 안해도 되는 처리까지 진행되고 있는 것 같아용!😮

Comment on lines +10 to +14
let menuItem = JSON.parse(localStorage.getItem("menu"));
if(!menuItem) {
menuItem = defaultMenuCategory
localStorage.menu = JSON.stringify(menuItem);
};

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
let menuItem = JSON.parse(localStorage.getItem("menu"));
if(!menuItem) {
menuItem = defaultMenuCategory
localStorage.menu = JSON.stringify(menuItem);
};
let menuItem = JSON.parse(localStorage.getItem("menu")) || defaultMenuCategory;
localStorage.menu = JSON.stringify(menuItem);

이렇게 변경해도 같은 동작을 할 것 같아요!

Comment on lines +20 to +22
let savedMenus = JSON.parse(localStorage.getItem('menu'));
savedMenus[category].push({name : menu});
localStorage.menu = JSON.stringify(savedMenus);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JSON.parse(localStorage.getItem('menu')JSON.stringify 가 중복해서 여러번 호출되고 있는데 함수화 하면 어떨까요~? 😀

const menu = fetchMenu();
const targetItem = menu[currentCategory][index];

let soldOut = !targetItem.hasOwnProperty('soldOut') ? true : targetItem.soldOut ? false : true;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

개인적으로 3항연산자를 2중으로 쓰게 되면 코드 읽기가 어려워지는 것 같아요 ㅠ

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants