-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
add task solution #983
base: master
Are you sure you want to change the base?
add task solution #983
Conversation
1luki9901
commented
Oct 13, 2024
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're doing a great job! 😊 However, there are a few areas that need some improvement for the code to be more efficient and user-friendly. Please consider the feedback and make the necessary changes. Keep up the good work! 👍
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
src/scripts/main.js
Outdated
const Game = require('../modules/Game.class'); | ||
|
||
// eslint-disable-next-line no-unused-expressions | ||
('use strict'); |
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.
The 'use strict' directive is usually placed at the beginning of a script, it's not working after the first executable statement.
src/scripts/main.js
Outdated
const Game = require('../modules/Game.class'); | ||
|
||
// eslint-disable-next-line no-unused-expressions | ||
('use strict'); | ||
|
||
// Uncomment the next lines to use your game instance in the browser | ||
// const Game = require('../modules/Game.class'); |
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.
This commented out line of code seems to be unnecessary since the Game class is already imported at the beginning of the file. It's a good practice to remove commented out code to keep your codebase clean.
src/scripts/main.js
Outdated
const state = game.getState(); | ||
const cells = gameBoard.getElementsByClassName('field-cell'); |
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.
It's generally a good practice to cache DOM elements that you'll need to reference multiple times. Consider storing the 'cells' collection in a variable outside of the 'updateUI' function. This way, you won't need to query the DOM every time 'updateUI' is called, which can improve performance.
src/scripts/main.js
Outdated
startButton.addEventListener('click', () => { | ||
if (game.getStatus() === 'playing') { | ||
game.restart(); | ||
} else { | ||
game.start(); | ||
} | ||
updateUI(); | ||
startButton.textContent = 'Restart'; | ||
}); |
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.
The click event listener on the 'startButton' is changing the button's text content to 'Restart' every time it's clicked. This could potentially be confusing to the user if the game is already in progress. Consider updating the button's text context based on the game's status.
src/scripts/main.js
Outdated
document.addEventListener('keydown', (event) => { | ||
let moved = false; | ||
|
||
switch (event.key) { | ||
case 'ArrowLeft': | ||
moved = game.moveLeft(); | ||
break; | ||
case 'ArrowRight': | ||
moved = game.moveRight(); | ||
break; | ||
case 'ArrowUp': | ||
moved = game.moveUp(); | ||
break; | ||
case 'ArrowDown': | ||
moved = game.moveDown(); | ||
break; | ||
} | ||
|
||
if (moved) { | ||
updateUI(); | ||
} | ||
}); |
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.
The 'keydown' event listener is currently set on the 'document'. This means that any key press will trigger this event, even when the user is not focused on the game. Consider setting the 'keydown' event listener on a specific element related to the game, to ensure that the game controls only function when the game is in focus.
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.
Great job so far! 😊 There are a few areas that need improvement for better performance and readability. Please consider the provided feedback and make the necessary changes. Keep going! 💪
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
src/scripts/main.js
Outdated
// const Game = require('../modules/Game.class'); | ||
|
||
// eslint-disable-next-line no-unused-expressions | ||
('use strict'); |
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.
The 'use strict' directive is not needed here since it's already inside a module. Modules are always in strict mode.
src/scripts/main.js
Outdated
function updateUI() { | ||
const state = game.getState(); | ||
|
||
for (let i = 0; i < cells.length; i++) { | ||
const cell = cells[i]; | ||
const value = state[Math.floor(i / 4)][i % 4]; | ||
|
||
cell.className = 'field-cell'; | ||
|
||
if (value) { | ||
cell.classList.add(`field-cell--${value}`); | ||
cell.textContent = value; | ||
} else { | ||
cell.textContent = ''; | ||
} | ||
} | ||
scoreElement.textContent = game.getScore(); | ||
statusElement.textContent = game.getStatus(); | ||
} |
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.
This updateUI
function seems to be doing a lot of work. It might be more efficient to only update the parts of the UI that have changed, rather than updating the entire UI every time.
src/scripts/main.js
Outdated
gameBoard.addEventListener('keydown', (event) => { | ||
let moved = false; | ||
|
||
switch (event.key) { | ||
case 'ArrowLeft': | ||
moved = game.moveLeft(); | ||
break; | ||
case 'ArrowRight': | ||
moved = game.moveRight(); | ||
break; | ||
case 'ArrowUp': | ||
moved = game.moveUp(); | ||
break; | ||
case 'ArrowDown': | ||
moved = game.moveDown(); | ||
break; | ||
} | ||
|
||
if (moved) { | ||
updateUI(); | ||
} | ||
}); |
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.
This event listener for keydown events on the gameBoard
element might not work as expected. The keydown event is typically listened for on the document
or window
object, not on specific elements, unless those elements are focused. You may want to reconsider where you're attaching this event listener.
src/scripts/main.js
Outdated
switch (event.key) { | ||
case 'ArrowLeft': | ||
moved = game.moveLeft(); | ||
break; | ||
case 'ArrowRight': | ||
moved = game.moveRight(); | ||
break; | ||
case 'ArrowUp': | ||
moved = game.moveUp(); | ||
break; | ||
case 'ArrowDown': | ||
moved = game.moveDown(); | ||
break; |
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.
The switch-case statement here could be simplified by using an object mapping instead. This would map each key to a corresponding function on the game
object, reducing the amount of code and improving readability.
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.
Demo link doesn't work.
src/scripts/main.js
Outdated
@@ -1,7 +1,69 @@ | |||
'use strict'; | |||
// const Game = require('../modules/Game.class'); | |||
|
|||
// Uncomment the next lines to use your game instance in the browser |
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.
Where is that file?
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.
The link you provided doesn't work. I noticed that the Game.class file is present, but there haven't been any changes made to it. You need to implement all the logic within that file to make the game functional.
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.
rerequest
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.
Hey. Good job with implementation, but I need working DEMO link so I can check if everything is working. You have to run npm run deploy
in the terminal.
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.
Let's resolve the problem on the Mate Academy chat, and once you successfully deploy, you can request a review again.
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.
Game doesn't work at all. After clicking start button nothing happens.
2048.-.Google.Chrome.2024-10-26.19-09-42.mp4No właśnie nie ogarniam czemu |