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

[2단계 - 자판기] 병민(윤병인) 미션 제출합니다. #66

Merged
merged 118 commits into from
Apr 14, 2022

Conversation

airman5573
Copy link

@airman5573 airman5573 commented Apr 7, 2022

안녕하세요 :D
이번 리뷰도 잘 부탁드립니다!

데모 페이지

Routing

<login-page data-to='["/login"]'></login-page>
<register-page data-to='["/register"]'></register-page>

이런식으로 attribute에 배열로 넘겨주면 RouteComponent를 상속받은 컴포넌트에서 shouldRender으로 현재 이 컴포넌트를 그려야 하는지 마는지 판단합니다.

그리고 Router클래스에서 location의 변화를 감지합니다.

window.addEventListener('load', this.onLocationChange);
window.addEventListener('pushstate', this.onLocationChange);
window.addEventListener('popstate', this.onLocationChange);

location에 변화가 생기면 RouteComponent를 상속받은 컴포넌트에 알람이 가서 render가 됩니다.

인증

heroku서버에 서버를 띄웠습니다. json-auth-server대신 간단히 json-server 와 lodash-id를 사용해서 구현했습니다.
/register, /login, /edit api를 지원합니다.

airman5573 and others added 30 commits March 23, 2022 15:39
commitlint + commitizen 을 사용해서 commit message를 정형화한다
single quote, EOL, trailingComma 적용
store, reducer, creaAction 생성
component scope를 추가한다
Copy link

@austinpark420 austinpark420 left a comment

Choose a reason for hiding this comment

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

안녕하세요 병민님.
명확한 기준을 가지고 컴포넌트를 쪼개서 작업을 잘 하신 것 같습니다.
네이밍과 구조 모두 일관성을 가지고 있어서 쉽게 코드를 이해할 수 있어서 좋았습니다.
또한 상황에 따라 토스트메세지 색을 다르게 준 것도 인상적이었습니다. 👍
로그인 시, 상품관리 탭이 active되지 않는 이슈가 있는데 수정하시면 좋을 것 같습니다.

@@ -0,0 +1,64 @@
import { testid } from '../support/utils';

Choose a reason for hiding this comment

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

command를 활용하지 않은 이유는 간단한 테스트코드만 작성하셔서 일까요?

Copy link
Author

Choose a reason for hiding this comment

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

미션 마감이 다가오고 command를 잘 쓸 자신이 없어서, 그냥 길게 작성했습니다. 이번에 수정하겠습니다 !

Copy link
Author

Choose a reason for hiding this comment

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

ba85bc5 수정했습니다 :D

docs/README.md Outdated
Comment on lines 58 to 96
- [ ] 로그인 버튼을 클릭하면 로그인 페이지가 보여진다.
- [ ] 이메일과 비밀번호를 입력해 로그인을 한다.
- [ ] 로그인하면 관리자가 된다.
- [ ] 로그인 하면 로그인 버튼이 관리자 이름으로 변경된다.
- [ ] 관리자 이름을 클릭하면 로그아웃 / 회원 정보 수정 버튼이 보여진다.
- [ ] 로그아웃 버튼을 클릭하면 로그아웃된다.
- [ ] 회원 정보 수정 버튼을 클릭하면 회원 정보 수정 페이지로 넘어간다.
- [ ] 회원가입 버튼을 클릭하면 회원가입 페이지가 보여진다.
- [ ] 회원가입을 할수 있다.
- [ ] 이름과 비밀번호를 입력한다.
- [ ] 이름은 2 ~ 6글자까지 가능하다.
- [ ] 이메일 형식이 맞아야 한다.
- [ ] 이미 가입된 이메일로 회원가입을 할 수 없다.
- [ ] password는 규칙에 맞게 입력해야한다.
- [ ] 8자 이상의 문자열 이어야 한다.
- [ ] 최소 하나 이상의 알파벳를 포함해야한다.
- [ ] 최소 하나 이상의 숫자를 포함해야 한다.
- [ ] 최소 하나 이상의 특수문자를 포함해야한다.
- [ ] 최소 하나 이상의 대문자 알파벳을 포함해야한다.
- [ ] 같은 문자를 3번 반복하면 안된다.
- [ ] 연속된 숫자가 3개를 초과하면 안된다.
- [ ] 한글로 입력해도 영어로 변환되어 입력된다.
- [ ] 회원가입을 하면 관리자 권한이 생긴다.
- [ ] 일반 유저 권한
- [ ] 로그인 버튼과 상품 구매 탭만 보여진다.
- [ ] 상품 구매 탭은 로그인여부에 관계없이 보여진다.
- [ ] 로그인 여부에 관계 없이 상품 구매 가능하다.
- [ ] 관리자 권한
- [ ] 상품 관리 탭이 보여진다.
- [ ] 관리자만 상품을 추가할 수 있다.
- [ ] 잔돈 충전 탭이 보여진다.
- [ ] 관리자만 잔돈을 충전할 수 있다.
- [ ] 상품 구매 탭이 보여진다.
- [ ] 회원 정보 수정
- [ ] 기존에 등록된 이메일과 이름이 placeholder에 보여진다.
- [ ] 이메일은 수정할 수 없다.
- [ ] 이름을 수정할 수 있다.
- [ ] 비밀번호를 수정할 수 있다.
- [ ] 바꾸려는 비밀번호를 입력하고, 그 아래에 한 번 더 입력해야 수정할 수 있다.

Choose a reason for hiding this comment

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

구현 하신것 같은데 체크 해주시면 좋을 것 같습니다.

Copy link
Author

Choose a reason for hiding this comment

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

앗, 수정하겠습니다!

Copy link
Author

Choose a reason for hiding this comment

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

05d212a 수정했습니다 :D

Comment on lines +11 to +21
<vendingmachine-page data-to='[
"/",
"/vending-machine",
"/vending-machine/product-manage",
"/vending-machine/charge-money",
"/vending-machine/purchase-product"]'>
</vendingmachine-page>
<login-page data-to='["/login"]'></login-page>
<register-page data-to='["/register"]'></register-page>
<my-account-page data-to='["/my-account"]'></my-account-page>
<side-toast></side-toast>

Choose a reason for hiding this comment

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

👍

</div>
</td>
</tr>
`;
}

template(productList: Array<ProductItem>): string {
const header = `<h2>상품 현황</h2>`;
if (productList.length === 0)

Choose a reason for hiding this comment

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

아래와 같은 형태는 어떠세요?

Suggested change
if (productList.length === 0)
if (!productList.length)

Copy link
Author

Choose a reason for hiding this comment

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

좋습니다, 수정하겠습니다 :D

Copy link
Author

Choose a reason for hiding this comment

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

f2436bb
수정했습니다 :D

Comment on lines 101 to 102
if (originalName === name && item.name === name) return false;
return true;

Choose a reason for hiding this comment

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

아래와 같이 작성하게되면 조금 더 직관적으로 알 수 있을 것 같은데 어떠세요?

Suggested change
if (originalName === name && item.name === name) return false;
return true;
return originalName === name && item.name === name;

Copy link
Author

Choose a reason for hiding this comment

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

더 깔끔합니다. 수정하겠습니다 :D

Copy link
Author

Choose a reason for hiding this comment

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

f2436bb
수정했습니다 :D

Comment on lines 111 to 146
onClickLoginBtn = async () => {
const feedbacks = this.validate();
this.setFeedbacks(feedbacks);
const hasError = (Object.keys(feedbacks) as Array<keyof FeedbackRecord>).some(
(key) => feedbacks[key].hasError
);
if (hasError) {
toast(ToastType.Error, '이메일 혹은 비밀번호입력에 오류가 발생했습니다');
this.resetPasswordInput();
return;
}
this.setIsLoading(true);
const [email, password] = [feedbacks.email.inputValue, feedbacks.password.inputValue];

try {
const response = await this.login({ email, password });
this.setIsLoading(false);
if (!response) throw new Error('통신에 오류가 발생했습니다');

const body = await response.json();

if (!response.ok) {
toast(ToastType.Error, body.errorMessage);
this.resetPasswordInput();
return;
}

const { accessToken, user, message } = body;
toast(ToastType.Success, message);
localStorage.setItem(ACCESS_TOKEN_KEY, accessToken);
localStorage.setItem(USER_INFO_KEY, JSON.stringify(user));
Router.pushState(WhiteList.Home);
} catch (e) {
console.error(e);
}
};

Choose a reason for hiding this comment

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

하나의 함수가 너무 많은 일을 하고 있는 것 같아 좀 작은단위로 리팩토링 가능하면 해보시는것도 좋을 것 같습니다.

Copy link
Author

Choose a reason for hiding this comment

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

네, 리팩토링 하겠습니다 :D

Copy link
Author

Choose a reason for hiding this comment

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

현재 코드 순서가 valiate -> error표시 -> 네트워크 통신 -> 결과 핸들링 이런식으로 되는데, 제가 보기에는 이 과정들이 너무 끈끈하게 엮여있어서 나누기가 어려워 보입니다. 리뷰어님의 도움이 필요합니다.. 어떻게 나누면 좋을까요?

Choose a reason for hiding this comment

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

아래 두 그룹을 하나의 함수로 작성하고, 네트워크 통신, 결과 핸들링의 함수에 feedbacks를 파라미터로 받는 식으로 리펙토링을 해보는 건 어떨까요? 병민님이 판단하셔서 리펙토링을 하지않는 것이 좋다고 판단하시면 그대로 두어도 무방할 것 같습니다. 😄

  • valiate, error표시
  • 네트워크 통신, 결과 핸들링

Copy link
Author

Choose a reason for hiding this comment

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

67e4d8c 말씀해주신 것을 참고해서 나누어 보았습니다 :D

@@ -0,0 +1,37 @@
import Component from '../abstract/component';

Choose a reason for hiding this comment

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

Component는 불필요한 import인거 같습니다.

Copy link
Author

Choose a reason for hiding this comment

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

맞습니다, 삭제하겠습니다 :D

Comment on lines +1 to +9
import Component from '../../abstract/component';
import { ACTION } from '../../constants';
import { customElement } from '../../decorators/decortators';
import createAction from '../../flux/createAction';
import Store from '../../flux/store';
import { ToastType } from '../../types';
import { consoleErrorWithConditionalToast, toast, toInt } from '../../utils';
import ValidationError from '../../validation/validation-error';
import { validateInsertMoney } from '../../validation/validators';

Choose a reason for hiding this comment

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

다음 프로젝트때, babel-plugin-module-resolver을 활용하셔서 절대경로로 세팅해보시는 것도 좋을 것 같습니다.

Copy link
Author

Choose a reason for hiding this comment

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

와..! 감사합니다 :D

@@ -0,0 +1,36 @@
import Component from '../abstract/component';

Choose a reason for hiding this comment

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

back-arrow.ts, logged_in-btn.ts처럼 모듈로 사용하는 클래드도 상위폴더를 하나 만들고 관리하면 가독성이 좀 더 좋을 것 같습니다.

Copy link
Author

Choose a reason for hiding this comment

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

감사합니다. 수정하겠습니다 :D

Copy link
Author

Choose a reason for hiding this comment

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

52e9045 수정했습니다 :D

그런데, 이렇게 하는게 맞나요? import를 한다 치면
import './components/login-btn/login-btn'; 이런 모양이 되는데,
반복되는 부분 때문에 개인적으로는 가독성이 좋아보이지 않습니다.
리뷰어님의 생각은 어떠신가요?

Choose a reason for hiding this comment

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

제 생각은 ./components/modules 하나의 디렉토리안에 모아두면 어떨까 말씀드렸던 부분이었습니다.
제가 모호하게 말씀드려서 죄송합니다. 🙏

Copy link
Author

Choose a reason for hiding this comment

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

6826b70 수정했습니다 :D

@airman5573
Copy link
Author

로그인 시, 상품관리 탭이 active되지 않는 이슈가 있는데 수정하시면 좋을 것 같습니다.

3eb8730 수정 했습니다 :D

리뷰어님! 말씀해 주신 것들은 대부분 수정 했지만, 아직 부족한게 많아서 조금 더 채우고 싶습니다. 테스트, 회원가입시 비밀번호 입력 규칙을 보여주기, toast animation 개선, 존재하지 않는 페이지로 이동할시 보여줄 404페이지 등등 방학동안 천천히 채워나가고 싶은데, 혹시 merge를 미뤄주실 수 있을까요?

비밀번호 입력시 어떤 규칙이 적용되야 하는지 사용자에게 알려주는 용도로 사용했다
@austinpark420
Copy link

austinpark420 commented Apr 13, 2022

네 알겠습니다. 머지가 필요하실 때 슬랙으로 DM주시면 확인 후 머지하도록 하겠습니다. 😄
포코에게는 제가 따로 이야기할게요.

Copy link

@austinpark420 austinpark420 left a comment

Choose a reason for hiding this comment

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

늦게까지 자판기 미션을 수행하시느라 고생 많으셨습니다. 👍
열정적으로 개발하시는 모습이 너무 인상적이었습니다.
방학이 벌써 얼마 남지는 않았지만 푹쉬시고 좋은 컨디션으로 다음 미션을 수행하시길 바랄게요!!!
이번 미션은 여기서 머지하도록 하겠습니다.

@austinpark420 austinpark420 merged commit a7ee26f into woowacourse:airman5573 Apr 14, 2022
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.

3 participants