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 상태 관리로 메뉴 관리하기 #287

Open
wants to merge 6 commits into
base: Leehaeun0
Choose a base branch
from

Conversation

Leehaeun0
Copy link

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

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

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.

안녕하세요 하은님!
다른팀이지만 리뷰 남겨 보았어요~! 😀

if (!name) return;

const menuId = getMenuId(target);
menuList = menuList.map((menu, index) => index === menuId ? {...menu, name} : menu);

Choose a reason for hiding this comment

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

menuList를 관리하는 객체를 만들어 보는건 어떨까요~? 😀

Comment on lines +62 to +63
const selectedCategory = target.dataset.categoryName;
category = selectedCategory;

Choose a reason for hiding this comment

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

Suggested change
const selectedCategory = target.dataset.categoryName;
category = selectedCategory;
category = target.dataset.categoryName;

이렇게 선언하지 않은건 selectedCategory 라는 변수 이름을 강조하고 싶으셨던건지 궁금해요~!

@@ -0,0 +1,18 @@
export default class LocalStorage {

Choose a reason for hiding this comment

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

스토리지가 깔끔하게 분리되어 있네요! 👍

Copy link

Choose a reason for hiding this comment

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

저는 단순히 util함수로 만들어서 로컬스토리지를 관리했는데 이 방법도 좋은것 같아요!👍


// states
// [{name: string; category: string; isSoldOut: boolean;}]
let menuList = MenuListStorage.get() ?? [];

Choose a reason for hiding this comment

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

?? 라는게 있었네요! 저는 || 를 쓰는데 앞으로는 ??를 써야겠어요! 감사합니다~!😀

let name = $id('menu-name').value?.trim();
if (!name) return;

menuList = [...menuList, {name, category}];

Choose a reason for hiding this comment

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

여기서
{name, category} 값만을 추가해주는 건
처음 생성한 메뉴의 default 상태가 품절이지 않고,
undefined 값일 경우 falsy 처리되기 때문일까요?

.map((menu, index) => {
return `
<li data-menu-id="${index}" class="menu-list-item d-flex items-center py-2">
<span class="w-100 pl-2 menu-name${menu.isSoldOut ? ' sold-out' : ''}">${menu.name}</span>

Choose a reason for hiding this comment

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

"sold-out" class 앞의 공백을 삼항연산자 안이 아니라
템플릿 리터럴 안에 넣으면 조금더 가독성에 좋을 것 같아요!

Suggested change
<span class="w-100 pl-2 menu-name${menu.isSoldOut ? ' sold-out' : ''}">${menu.name}</span>
<span class="w-100 pl-2 menu-name ${menu.isSoldOut ? 'sold-out' : ''}">${menu.name}</span>

Copy link

@bingwer bingwer left a comment

Choose a reason for hiding this comment

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

안녕하세요 하은님! 이번주는 리뷰가 조금 늦었습니다. 😂
고생 많으셨어요!!

@@ -0,0 +1,18 @@
export default class LocalStorage {
Copy link

Choose a reason for hiding this comment

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

저는 단순히 util함수로 만들어서 로컬스토리지를 관리했는데 이 방법도 좋은것 같아요!👍

@@ -0,0 +1,3 @@
export const $ = (selector) => document.querySelector(selector);

export const $id = (id) => document.getElementById(id);
Copy link

Choose a reason for hiding this comment

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

$(#id)나, $(.class) 로 위의 선언하신 두가지 유틸함수를 사용할 수 있을것 같습니다!
혹시 두가지를 나누신 이유는 성능상의 차이를 구분하기 위해서 나누신 걸까요?

if (!window.confirm('정말 삭제하시겠습니까?')) return;

const menuId = getMenuId(target);
menuList = menuList.filter((_, index) => index !== menuId);
Copy link

Choose a reason for hiding this comment

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

menuList를 변경하고, 로컬스토리지에 저장하는 작업이 반복적으로 사용이 되고있는데요, 함수로 따로 관리한다면 더 좋을것 같아요!

const toggleIsSoldOut = ({target}) => {
const menuId = getMenuId(target);
menuList = menuList.map((menu, index) =>
index === menuId ? {...menu, isSoldOut: !menu.isSoldOut} : menu,
Copy link

@bingwer bingwer Jul 29, 2022

Choose a reason for hiding this comment

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

editMenu(), removeMemu(), toggleIsSoldOut() 이 함수들에서
menuList를 변경하는부분에서 category를 확인하는 부분이 빠져있는데요.
이렇게되면 각 카테고리별로 중복된 이름의 메뉴가 존재한다면 해당 함수의 작업들이 사용자가 원하는 카테고리의 메뉴가 아니라
menuList에서 index가 가장 빠른 메뉴가 수정, 삭제, 품절이되는 버그가 발생할수 있을것 같아요!

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.

4 participants