Conversation
ElizabethSh
left a comment
There was a problem hiding this comment.
Looks good! 👍
Areas for improvement:
- You overcomplicate your code (see comments). Make it simple and easier to read.
- You use inline styles a lot instead of using
css. It also make your code difficult to read. Please usecssinstead of inline styles. - You use
jsfor layout but you can use media queries incssand change layout depending on breakpoint. - Please create separate callback functions and use them in event listeners. It makes code easier to read.
- Please don't use function before it's initialization.
- Please create variables for 'magic numbers'.
| } | ||
|
|
||
| .level-btn { | ||
| background: #10574d; |
There was a problem hiding this comment.
You could create variables for colors. Example:
:root {
--primary-color: #3498db;
}
button {
background-color: var(--primary-color);
}
There was a problem hiding this comment.
Thank you for your feedback! 😊
We’ve created CSS variables for all main colors and fonts in the code, so it’s now easier to maintain and adjust.
|
|
||
| const cardFrontImageSrc = "Images/lotus-flower.png"; | ||
|
|
||
| const levelSettings = { |
There was a problem hiding this comment.
what are these magic numbers 6, 8, 10 and 60, 90, 120?
There was a problem hiding this comment.
Hi Lisa!
We’ve implemented the requested changes to remove magic numbers.
| } | ||
|
|
||
| secondCard = card; | ||
| checkForMatch(); |
There was a problem hiding this comment.
Please don't use function before initialization.
There was a problem hiding this comment.
We reordered functions so checkForMatch() is fully defined before flipCard() calls it.
| } | ||
|
|
||
| function setGridColumns(numberOfPairs = defaultNumberOfPairs) { | ||
| const isMobile = window.innerWidth <= 600; |
There was a problem hiding this comment.
What is 600? Please create variable for it.
There was a problem hiding this comment.
Thank you for highlighting this. We implemented another solution using CSS:
.container {
grid-template-columns: repeat(4, 1fr);
}
@media (min-width: 600px) {
.container {
grid-template-columns: repeat(var(--columns, 4), 1fr);
}
}
| let columns; | ||
|
|
||
| if (isMobile) { | ||
| columns = 4; | ||
| } else { | ||
| if (numberOfPairs <= 8) columns = 4; | ||
| else if (numberOfPairs <= 10) columns = 5; | ||
| else columns = 6; | ||
| } |
There was a problem hiding this comment.
This looks too complicated and formatting is off. You could define default value for columns e.g.
let columns = 4;
and then change it in if statements. In this case you don't need to use 1 if.
There was a problem hiding this comment.
I see what you mean. We improved the formatting:
function setGridColumns(numberOfPairs = defaultNumberOfPairs) {
let columns = 4;
if (numberOfPairs === 10) columns = 5;
gameBoard.style.setProperty("--columns", columns);
}
| btn.addEventListener("click", () => { | ||
| currentPairs = parseInt(btn.dataset.pairs, 10); | ||
|
|
||
| levelButtons.forEach((level) => level.classList.remove("active")); |
There was a problem hiding this comment.
Why you use levelButtons inside 'specific' button?
There was a problem hiding this comment.
We extracted the active state logic into a dedicated updateActiveLevel() function so the button handler only manages state changes and delegates the group UI update.
| winner.style.display = "flex"; | ||
| } | ||
|
|
||
| document.addEventListener("DOMContentLoaded", () => { |
There was a problem hiding this comment.
I encourage you to create a separate function for this callback.
There was a problem hiding this comment.
We extracted the DOMContentLoaded callback into an initializeGame() function to improve readability and keep the event listener focused only on triggering initialization.
| createGame(currentPairs); | ||
| }); | ||
|
|
||
| window.addEventListener("resize", () => { |
There was a problem hiding this comment.
You could just use media queries in css and change layout depending on breakpoint. In this case you don't need setGridColumns function.
There was a problem hiding this comment.
Thank you for the suggestion! Since we solved the setting of the column number for mobile through CSS, the eventListener is not needed and was deleted.
In our solution, number of columns depends on the screen size as well as on number of card pairs:
let columns = 4; (for 6 and 8 pairs)
if (numberOfPairs === 10) columns = 5;
| lockBoard = true; | ||
|
|
||
| setTimeout(() => { | ||
| firstCard.style.transform = "scale(0.5) rotateY(180deg)"; |
There was a problem hiding this comment.
Why you don't do it in css? You could create class fr that and add or remove it depending on condition.
There was a problem hiding this comment.
Thank you for pointing that out. We’ve moved this property to CSS.
|
Hi @ElizabethSh, Thank you so much for taking the time to review our pull request. Your feedback was really helpful. You pointed out exactly the areas where we had some doubts — like using CSS media queries instead of JS, the “magic numbers,” and refactoring the setGridColumns(). |
b34b5e7 to
af4c48f
Compare
Co-authored-by: Lisa Shoshkina <liza.shoshkina@gmail.com>
| if ( | ||
| lockBoard || | ||
| card === firstCard || | ||
| card.classList.contains("flipped") || | ||
| card.classList.contains("matched") | ||
| ) |
There was a problem hiding this comment.
This if statement block is duplicated here
There was a problem hiding this comment.
I checked the current version, and the condition only exists inside flipCard().
I’ve grouped it into a single conditional for clarity.
| } | ||
|
|
||
| function showTimeout() { | ||
| lockBoard = true; |
There was a problem hiding this comment.
I think lockBoard = true isn't doing anything here, because resetBoard() flips it immediately to false again.
There was a problem hiding this comment.
Good catch!! resetBoard() was implicitly flipping lockBoard back to false, which made lockBoard = true inside showTimeout() ineffective.
We refactored resetBoard() to clear only the selected cards, and now explicitly manage lockBoard in unflipCards(), createGame(), and disableCards().
| resetBoard(); | ||
| }; | ||
|
|
||
| secondCard.addEventListener("transitionend", handleTransitionEnd); |
There was a problem hiding this comment.
Tip for future projects :) - Don't need to change this now:
I can see why this works here, but I would encourage to not tie logic to animations changes.
The problem is that transitionend is not always "guaranteed" to fire.
- There are browser settings like "prefer reduced motion" that will make animations not execute.
- Battery saver (low battery on mobile devices, fx.) can cause animations to be skipped
- other things like changing the focused tab to another tab in your browser might also make this event not run.
An alternative tradeoff here (less "elegant", but more reliable) would be to use a setTimeout that matches the duration of your expected animation.
There was a problem hiding this comment.
Hi Jesús,
Thanks for the tip! We actually refactored the card-flipping logic in the current version. Instead of relying on transitionend, we now use setTimeout with the duration matching the expected animation (flipBackDelay for unflipping cards and matchedDelay for matched cards). This way, the game flow and resetBoard() work reliably even if animations are skipped due to browser settings, battery saver, or reduced motion preferences.
Just to let you know, your concern about transitionend not firing is already addressed in this version. 😊
Thanks for the guidance!
There was a problem hiding this comment.
@jesusoterogomez thank you for explaining the drawbacks of this approach and for suggesting a more appropriate solution!
There was a problem hiding this comment.
When I used the transitionend for this functionality, my reasoning was that relying on setTimeout and hardcoding it to match the CSS animation duration could make the logic fragile. For example, if the animation duration in CSS is 0.8 seconds, we would set the timeout to 800ms. But if someone later changes the animation duration in CSS to 1 second, the JavaScript logic would no longer be in sync and could break. That’s why I preferred using transitionend, so the logic reacts to the actual end of the animation rather than a fixed time value. @jesusoterogomez could you please share your thoughts on this?
There was a problem hiding this comment.
Your logic makes sense here, but the tradeoff is that matching it to the CSS makes it easier to modify in one place, but at the cost of reliability.
Commonly, in bigger projects you would use an "animation library", that controls all animations via JS, which would eliminate this issue of matching JS and CSS variables.
I'm thinking of alternate approaches, and I think it's possible to solve it with JS and CSS variables:
I haven't tried this myself, but I believe this works, and it could be a neat and pretty elegant solution
- Define your time variable in JS and set it as CSS variable.
// animation duration in ms
const durationInMs = 800;
const card = ... // reference to the element in HTML;
// Set the CSS variable property
card.style.setProperty(
"--flip-duration",
durationInMs + "ms"
);- Use the property from the variable in CSS
.card {
transition: transform var(--flip-duration) ease;
}- Use the same JS variable for the timeout
...
setTimeout(() => {...}, durationInMs);
...That would make the same values match in CSS and JS, and only defined in one place.
| @@ -0,0 +1,94 @@ | |||
| # Postman – Test Scripts | |||
There was a problem hiding this comment.
Awesome that you included tests for your endpoints :)
Later, you'll also learn about testing frameworks and how you can do this within your project.
There was a problem hiding this comment.
Thanks, Jesús! Looking forward to learning more about testing frameworks and integrating them directly into the project.
Hi, dear mentor!
In this PR, we have implemented the assignments according to Milestones 2 and 3 for the Memory game project.
What has been done:
Game logic implemented:
Additional features implemented:
Looking forward to your review.
Kind regards,
Paloma & Iryna