-
-
Notifications
You must be signed in to change notification settings - Fork 184
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
Memory pair game #384
base: main
Are you sure you want to change the base?
Memory pair game #384
Conversation
…user select: none
This issue has been automatically marked as stale because there were no activity during last 14 days. It will be closed in 7 days if no further activity occurs. Thank you for your contributions. А. Чому так? Б. Що робити, якщо в піарі нема оновлень, оскільки не зрозуміло, що треба зробити? В. А якщо я все зробив(ла) і це ментор не рев'юває мої зміни?
Г. Хіба недостатньо того, що я додав(ла) коміт із змінами? Традиційна пропозиція: задай питання по вищенаписаному в студентському чаті. |
Nooooo, bot, I'm not stale, I'm still here ! |
This issue has been automatically marked as stale because there were no activity during last 14 days. It will be closed in 7 days if no further activity occurs. Thank you for your contributions. А. Чому так? Б. Що робити, якщо в піарі нема оновлень, оскільки не зрозуміло, що треба зробити? В. А якщо я все зробив(ла) і це ментор не рев'юває мої зміни?
Г. Хіба недостатньо того, що я додав(ла) коміт із змінами? Традиційна пропозиція: задай питання по вищенаписаному в студентському чаті. |
I'm still here... |
UAT - done. Feel myself ready for review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good work!
|
||
function cardsHandler() { | ||
const cards = document.querySelectorAll(".card"); | ||
cards.forEach((card) => card.addEventListener("click", flipCard)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
User event delegation. Read more
if (lockBoard) return; | ||
if (this === firstCard) return; | ||
|
||
this.classList.add("flip"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can easily lose context and the corresponding value of this
pass selected card as a parameter for the function.
…refactored game logic, small improvements
…of modal window on top, when new modal appears - changes only text and btn title
Good day. Fixed everuthing you mentioned, applied Event delegation to gameboard, so slightly reefactored game logic, some tricks with event targets. Also refactored modal rendering, put out making of modal from function, in func changing only modal text and button title. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good progress!
function cardsHandler() { | ||
gameboard.addEventListener("click", ({ target }) => { | ||
if ( | ||
target.parentElement.classList.contains("card") && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't use constructions like children[0], firstElementChild, parentNode, nextSibling, etc. In such way, you rely on the order of DOM elements. So in case when you will have changed design - your code will be broken. Which is bad, obviously. Use querySelector or closest, if in event, instead. Read more
prepareGame(); | ||
} | ||
|
||
function prepareGame() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your function calls prepareGame
but inside this function, you describe all game logic.
Single responsibility - your functions should do only one job. As an example function in which you fetch users should only fetch them and not render, transform or process them in other ways. All these things should be done in another place, outside your function. The same applied to the sort function, which usually does all types of sorting 😉
…ering pf gameboard outside of function, now changing only score number on page
Good day. Applied target.closest to define clicked card. Also refactored prepare game function, splited it to func, that renders gameboard, func, that handles card click and resposible for game logic, and separetely pulled out rendering of scoreboard. Now it's rendered only once and then changing only one number of score. Previosly scoreboard was deleted totally and re-rendered everytime game resets. Which, I think, is not ok. One question about your comment: "The same applied to the sort function" ? Do I need to make sorting a separate function? I do it in prepareGameBoardLayOut(), just because it is used only in that place and nowhere else, so I leaved it here, not to mess up other code by one more function. |
This issue has been automatically marked as stale because there were no activity during last 14 days. It will be closed in 7 days if no further activity occurs. Thank you for your contributions. А. Чому так? Б. Що робити, якщо в піарі нема оновлень, оскільки не зрозуміло, що треба зробити? В. А якщо я все зробив(ла) і це ментор не рев'юває мої зміни?
Г. Хіба недостатньо того, що я додав(ла) коміт із змінами? Традиційна пропозиція: задай питання по вищенаписаному в студентському чаті. |
Memory pair game
Demo |
Code base
The code is submitted in a dedicated feature branch.
Only code files are submitted.
Please, review.