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단계] 곽가영 미션 제출합니다. #10

Open
wants to merge 4 commits into
base: gykwak03
Choose a base branch
from

Conversation

gykwak03
Copy link

@gykwak03 gykwak03 commented Jan 14, 2024

안녕하세요.

한가지 이상의 테스트를 추가하는 것 외에는 모두 완료하였습니다. 좀 더 공부해서 테스트 추가하겠습니다.

감사합니다.

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.

안녕하세요 가영님~
여행은 잘 다녀오셨나요?

이번 2단계 미션 코드를 보며 확실히 자바스크립트 실력이 많이 늘고있다는 게 느껴집니다!😀
짧은 기간 내에 코드를 작성하는 게 쉽지않았을텐데 작성을 잘 해주셨네요

코멘트 읽어보시고 피드백 사항 반영해주세요!
꼭 제 말이 무조건 옳은건 아니니 같이 이야기 나눠보며 수정해보면 좋을 것 같습니다~

Comment on lines +23 to +24
display: flex;
gap: 10px;
Copy link
Contributor

Choose a reason for hiding this comment

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

중앙 배치가 되어있지 않은데 align-items과 justify 속성에 대해서 알아볼까요?

Copy link
Author

@gykwak03 gykwak03 Jan 16, 2024

Choose a reason for hiding this comment

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

align-items: center; 를 추가하면 가로축을 기준으로 카드들이 정렬하게 되고,
justify-content: center; 를 추가하면 세로축을 기준으로 가운데 정렬하게 됩니다!
코드에 삽입하겠습니다.

src/Card.js Outdated
this.gameOver = false;
this.information = document.createElement("p");
this.information.classList.add("information");
document.body.appendChild(this.information);
Copy link
Contributor

Choose a reason for hiding this comment

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

document.body에 삽입시키는 것보다 특정 html요소(cards 등) 에 삽입하는게 구조적으로 좋답니다!

Copy link
Author

Choose a reason for hiding this comment

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

넵 수정했습니다!

src/Card.js Outdated
Comment on lines 29 to 42
button.addEventListener("click", () => {
if (this.gameOver) return;

if (randomNum === index) {
button.textContent = "당첨";
this.endGame("(성공!) 당첨되었습니다.");
} else {
button.textContent = "꽝";
this.chance = this.chance - 1;

if (this.chance === 0) {
this.endGame("(실패!) 게임이 종료되었습니다.");
} else {
this.startGame(button, cardsContainer, randomNum, index);
Copy link
Contributor

Choose a reason for hiding this comment

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

if else문이 중첩되다보면 가독성이 떨어집니다.
해당 메소드를 나누어 메소드의 기능을 분리시키는 게 좋을 것 같아요!

Copy link
Author

Choose a reason for hiding this comment

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

넵 게임 종료 메시지를 밖으로 빼겠습니다!

restart.textContent = "재시작";
restart.classList.add("restart");
restart.addEventListener("click", () => {
location.reload(true);
Copy link
Contributor

Choose a reason for hiding this comment

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

버튼을 클릭했을 때 새로고침을 함으로써 재시작이 되도록 기능을 작성해주셨네요!
좋은 아이디어입니다. 하지만 제가 의도드린 사항은 start메소드를 다시 재사용해보는 것이었습니다. 시간이 된다면 다시 작성해보시는걸 추천드려요!

Copy link
Author

Choose a reason for hiding this comment

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

start 메소드를 end에서 재활용을 해보려 했는데 재시작과 변수 변경 등등이 오류가 나서.. 좀만 시간을 두고 다시 해보겠습니다 ㅠㅠ

src/index.js Outdated
Comment on lines 17 to 18
buttons.forEach((button, index) => {
gameManager.startGame(button, cardsContainer, randomNum, index);
Copy link
Contributor

Choose a reason for hiding this comment

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

카드 버튼에 이벤트를 주입하는 코드인데요 해당 기능은 gameManager 클래스가 소유하고 있는게 적절하다고 생각됩니다.
gameManager 클래스에서 buttons를 소유하고 해당 코드를 실행시키도록 해보는건 어떨까요?

Copy link
Author

Choose a reason for hiding this comment

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

넵 새로운 함수 만들어서 해보겠습니다!

this.gameOver = true;
this.information.textContent = message;

let restart = document.createElement("button");
Copy link
Contributor

Choose a reason for hiding this comment

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

button과 div element의 차이점은 무엇일까요?

Copy link
Author

Choose a reason for hiding this comment

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

button은 사용자가 클릭하면 동작하는 상호작용이 가능한 엘리먼트이고
div는 구간을 나누는 엘리먼트로써 어떠한 의미를 가지진 않지만 복합적인 컨테이너로 사용됩니다.

src/index.js Outdated
Comment on lines 6 to 13
const card1 = new Card(cardsContainer); // cardsContainer를 생성자에 전달
const card2 = new Card(cardsContainer); // cardsContainer를 생성자에 전달
const card3 = new Card(cardsContainer); // cardsContainer를 생성자에 전달
const card2 = new Card(cardsContainer);
const card3 = new Card(cardsContainer);
const card4 = new Card(cardsContainer);

const buttons = document.querySelectorAll("button");

const randomNum = Math.floor(Math.random() * 3);
const randomNum = Math.floor(Math.random() * 4);
Copy link
Contributor

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.

넵 수정하겠습니다!

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