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

[1단계 - 행운의 로또 미션] 미키(권세진) 미션 제출합니다 #18

Merged
merged 24 commits into from Feb 21, 2021

Conversation

0307kwon
Copy link

1단계 - 행운의 로또 미션 🍀

데모 사이트

안녕하세요 @iborymagic 와 페어를 진행한 미키입니다! 🐭
저희는 이번 로또 미션 프로젝트가 규모가 작기 때문에
MVC 모델을 적용해야 할 필요를 느끼지 못해 의견을 나눈 끝에
controller(비즈니스 로직) + ui 형태로 웹을 구성하였습니다.
저희는 규모가 작아서 이런 방식이 괜찮다고 생각하였는데 리뷰어님의 의견이 궁금합니다ㅎㅎ

또 저희가 함수 이름에 대해서 고민하는 시간이 많았는데요
특히 아래와 같은 함수 이름에 대해서 고민이 많았습니다.

export function isMoneyUnderLottoPrice(money) {
  return money < LOTTO_SETTINGS.LOTTO_PRICE;
}

로또를 최소 한 장은 구입할 수 있어야하므로
로또 가격보다 입력 금액이 작은지를 판단하는 함수입니다
혹시 리뷰어님이시라면 어떤 함수 이름을 지으셨을지 궁금합니다 😃

리뷰해주셔서 감사합니다! ㅎㅎ

- fix
	- 구입 금액을 잘 못 입력해도 다음 단계로 넘어가던 버그 수정
- refactor
	- 숫자와 alert 메세지 상수화
- 폴더명 일관성 있게 변경
@0307kwon 0307kwon changed the base branch from main to 0307kwon February 18, 2021 04:01
Copy link

@JaeYeopHan JaeYeopHan left a comment

Choose a reason for hiding this comment

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

1차 코멘트 남겼습니다!
구조가 잘 잡혀있어서 리뷰하기 좋았습니다 ㅎㅎ

package.json Outdated Show resolved Hide resolved
display: none;
}

.lotto-ticket {

Choose a reason for hiding this comment

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

css class naming은 BEM이라는 방법론에 대해 알아봐도 좋을 것 같아요!

Copy link
Author

@0307kwon 0307kwon Feb 21, 2021

Choose a reason for hiding this comment

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

BEM방식이 관리하기 쉬운 방식인 것 같네요! 감사합니다 👍

src/js/index.js Outdated
$modalClose.addEventListener('click', onModalClose)
const lottoUI = new LottoUI();
const lottoController = new LottoController(lottoUI);
lottoController.init();

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.

빨간 표시가 어떤 부분을 말씀하시는지 이해를 못했습니다 ㅠ 자세히 알 수 있을까요?! 😢

Choose a reason for hiding this comment

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

file changed에서 보시면 보일거예요!

image

Copy link
Author

Choose a reason for hiding this comment

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

헉 저 표시가 경고 문구였다는 사실을 몰랐었네요!
항상 파일 끝에는 eol을 넣어주는 것이 맞군요.
새로운 사실을 알게 되었습니다. 감사합니다! 👍

src/js/utils/validation.js Outdated Show resolved Hide resolved
src/js/lottoController.js Show resolved Hide resolved
Copy link
Author

@0307kwon 0307kwon left a comment

Choose a reason for hiding this comment

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

리뷰 감사합니다!
한 가지 이해를 못한 부분이 있어 질문드렸습니다 ㅎㅎ
(#18 (comment))
좋은 하루 보내세용! 😊

[CSS class Naming] {BEM} - 3

내용

  • BEM은 css class의 이름을 좀 더 관리하기 쉬운 형태로 만들기 위한 방법론임
  • 네이밍은 Block__Element--Modifier로 표현됨. 각 요소는 하이픈(-)으로 긴 문자열을 표현할 수 있음
  • BEM은 Block, Element, Modifier의 약자임
    • Block : 재사용 할 수 있는 기능적으로 독립적인 페이지 구성 요소
    • Element : block에 종속되어 있는 요소로, 어떤 목적을 가지고 있는지가 드러나야 함
    • Modifier : 다른 Block이나 Element와 거의 같지만 다른 성질이나 상태를 가지고 있는 경우.
      • boolean type : 수식어의 값이 true 라고 가정한다.
        (ex : form__button — disabled)
      • key-value type : key, value를 하이픈으로 연결하여 성질 - 내용을 표시한다.
        (ex : color-red, theme-ocean)

링크

[기타] {코드 가독성} - 2

내용

  • 함수 이름을 너무 짓기 힘들면 해당 함수가 정말 필요한지 다시 고민해보기
  • click 이벤트에만 집착하지 말고, tag에 맞는 적절한 이벤트 고려하기

[기타] {모든 파일의 끝에 eol(개행문자)을 넣어줘야 하는 이유} - 2

내용

  • 옛날 IEEE에서 한 개의 line에 대해 다음과 같이 정의 했기 때문
    0 또는 개행문자(newline)가 아닌 문자들이 나오다가 개행문자(newline)로 끝이 나는 시퀀스
  • 현대에 사용되는 컴파일러들이 파일 끝에 eol이 없어서 컴파일이 안되는 경우는 없음
  • 하지만 내가 의도한 바와 다른 결과가 나올 수 있기 때문에 되도록 eol를 넣어주는 것이 좋다.

링크

display: none;
}

.lotto-ticket {
Copy link
Author

@0307kwon 0307kwon Feb 21, 2021

Choose a reason for hiding this comment

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

BEM방식이 관리하기 쉬운 방식인 것 같네요! 감사합니다 👍

src/js/lottoController.js Show resolved Hide resolved
src/js/utils/validation.js Outdated Show resolved Hide resolved
src/js/index.js Outdated
$modalClose.addEventListener('click', onModalClose)
const lottoUI = new LottoUI();
const lottoController = new LottoController(lottoUI);
lottoController.init();
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

@JaeYeopHan JaeYeopHan left a comment

Choose a reason for hiding this comment

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

수고하셨습니다!ㅎㅎ 👏 👏 👏

@JaeYeopHan JaeYeopHan merged commit 1f58d58 into woowacourse:0307kwon Feb 21, 2021
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.

None yet

2 participants