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단계] 이수진 미션 제출합니다. #11

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

sujin0707
Copy link

No description provided.

@tkdrb12
Copy link
Contributor

tkdrb12 commented Jan 15, 2024

image

css 깨짐 현상이 발생합니다
cards 요소를 고정 너비로 설정하고 padding, margin값을 주는 것을 추천합니다.

@tkdrb12
Copy link
Contributor

tkdrb12 commented Jan 15, 2024

이미 뒤집은 카드를 다시 클릭하거나 게임이 종료되었을 때 카드를 추가로 클릭해도 이벤트가 작동합니다.
개선을 해보시는 걸 추천드려요!

@tkdrb12
Copy link
Contributor

tkdrb12 commented Jan 15, 2024

시안에 맞게 재시작 버튼 css를 수정해주세요! absolute를 사용해주시면 되겠습니다

@tkdrb12
Copy link
Contributor

tkdrb12 commented Jan 15, 2024

재시작 버튼을 누를 시 Cannot read properties of undefined (reading 'ifLoser')와 같은 오류가 발생합니다!

Copy link
Contributor

@tkdrb12 tkdrb12 left a comment

Choose a reason for hiding this comment

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

안녕하세요 수진님!

클래스를 잘 활용해주시려 한 노력이 보이는 코드였습니다!
특히 커밋 내역을 신경써서 달아주셔서 좋았어요😀

하지만 미션 난이도가 있어 그런지 예외 케이스에 대한 대처가 잘 되어있지 않았는데요
코멘트 읽어보시고 피드백 반영부탁드립니다!

index.css Outdated
Comment on lines 28 to 30
margin-top: 10px;
margin-left: auto;
margin-right: auto;
Copy link
Contributor

Choose a reason for hiding this comment

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

margin 속성 값으로 하나로 축약해서 나타낼 수 있습니다!

Copy link
Author

Choose a reason for hiding this comment

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

힙쳐서 나타냈습니다!

src/CardGame.js Outdated
start() {
this.cards.forEach((card) => {
card.element.addEventListener('click', () => {
card.handleCardClick(this.resultContainer);
Copy link
Contributor

Choose a reason for hiding this comment

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

handleCardClick에는 인자가없는데 this.resultContainer를 사용한 이유가 있을까요?

Copy link
Author

Choose a reason for hiding this comment

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

아마 클릭 시 당첨 / 꽝이라는 글자가 카드에 생겨나야 하기 때문에 해당 인자를 전달해야 하는 것으로 잘못 착각한 것 같습니다. 수정하였습니다

src/CardGame.js Outdated
Comment on lines 65 to 66
this.cards.forEach((card) => {
card.element.removeEventListener('click', card.handleCardClick);
Copy link
Contributor

Choose a reason for hiding this comment

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

해당 함수는 제대로 동작하지 않을겁니다!
removeEventListener 의 인자로는 addEventListener와 같은 함수를 전달해야 이벤트를 제거할 수 있는데
bind함수는 매번 새로운 함수를 생성하기 때문입니다

예를 들어

const f1 = method.bind(this);
const f2 = method.bind(this);

f1, f2은 서로 다른 함수라고 할 수 있습니다

src/CardGame.js Outdated

createCards(game) {
const cards = [];
const winnerLocation = Math.floor(Math.random() * 3);
Copy link
Contributor

Choose a reason for hiding this comment

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

카드는 4개로 추가되었는데 랜덤값은 수정이 되어있지 않네요ㅜㅠ

Copy link
Author

Choose a reason for hiding this comment

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

랜덤값 수정하였습니다

@sujin0707
Copy link
Author

commit할 때 '저장 및 commit 하시겠습니까?' 이 팝업창에 yes를 클릭했더니 Card.js와 CardGame.js가 실수로 같이 커밋되어서 뒤집은 카드를 다시 클릭하거나 게임이 종료되었을 때 카드를 추가로 클릭해도 이벤트가 작동하는 부분 개선한 것과 Card.js 부분 개선한 것이 같이 푸시되었습니다 ㅠㅠ..!

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