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

Game page #49

Open
wants to merge 8 commits into
base: development
Choose a base branch
from
Open

Game page #49

wants to merge 8 commits into from

Conversation

carloshs1994
Copy link
Owner

Hi @JohnFTitor πŸ‘‹πŸ»

Thanks for taking the time to review the project. Here are some things important to note about it:

Sass

The project was built using Sass. The only difference was that we needed to add an additional loader to webpack, but the result is overall the same (A CSS output). Additionally, we used MiniCssExtractPlugin to generate an external CSS file in the dist folder instead of the styles in the head of the document. It has the same dynamic response, it's just that the output looks more organized.

Now into the project...

Changes implemented in "game-page" branch

  • βœ… My code follows the style guidelines of this project.
  • βœ… The mix and match game follows Fisher-Yates Shuffle Algorithm
  • βœ… We use the pokeapi to create the images for the cards inside the board
  • βœ… We added animations and transitions to the cards to make them more interactive
  • βœ… We used a class to store all the game's logic

Final notes

Is expected that this project meets the requirements. Please let us know if anything is left to be done or can be improved. Again, thanks for the review.

@carloshs1994 carloshs1994 self-assigned this Feb 7, 2022
@JohnFTitor JohnFTitor linked an issue Feb 7, 2022 that may be closed by this pull request
Comment on lines +177 to +196
const preGame = (cards) => {
const overlays = [...document.querySelectorAll('.overlay-text')];
const game = new PokemonGame(100, cards);
overlays.forEach((overlay) => {
overlay.addEventListener('click', () => {
overlay.classList.remove('visible');
game.startGame();
});
});
cards.forEach((card) => {
card.addEventListener('click', () => {
game.flipCard(card);
});
});
};

export default (arr) => {
const cards = printGame(arr);
preGame(cards);
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be nice to add a function here that allows the player to see the cards before they're hidden. That way we would allow them to win easily πŸ’―

Copy link
Owner Author

Choose a reason for hiding this comment

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

Hey @JohnFTitor! This is the first time I'm reading this comment, sorry about that. I think this one, in particular, is not something I would like to change because I actually like that you have to flip some cards before you understand what you are looking for. Honestly, I wanted to use random images from the first 200 pokemon so we could have a harder game but I didn't have the time last week. What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hello @carloshs1994
Don't worry. I see your point. Maybe I didn't express myself correctly.
The game should be like that of course. The user needs to flip cards in order to match them.

Question. Is this a memory game?
I thought it was a memory game because in that case, users need to first have a short, quick glance, to see all the cards flipped, before they hide again so the game starts. Because the way it's implemented right now makes you have to be wrong first in order to match them all.

If it was not intended as a memory game, then my bad. Please let me know which is the implementation you want πŸ’―
I agree. It would be a good idea. You can use this function

const getPokemons = async (startingIndex, numberOfPokemons) => {
let pokemons = [];
const finalIndex = (startingIndex + numberOfPokemons) >= 500 ? 500
: startingIndex + numberOfPokemons;
for (let i = startingIndex + 1; i <= finalIndex; i += 1) {
const pokemon = fetch(`https://pokeapi.co/api/v2/pokemon/${i}/`).then((response) => response.json());
pokemons.push(pokemon);
}
pokemons = await Promise.all(pokemons);
return pokemons;
};

To get the data within indexes. Please let me know if you have another idea.
Cheers!

Copy link
Collaborator

@JohnFTitor JohnFTitor left a comment

Choose a reason for hiding this comment

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

Hi @carloshs1994 πŸ‘‹πŸ»

I loved how the game looks. You did a great job! πŸŽ‰ πŸŽ‰
There are some things I think could be improved with the project. Please let me know if you think differently and have any questions. 😸

You've done a great job so far! Let's highlight the best approaches within this project:

  • βœ… Use of ES6 syntax creating a clean codebase. Especially organizing the functionality in a class. Brilliant.
  • βœ… Dynamically Generate the content of the page following the SPA
  • βœ… The game is awesome and fun. You did a great job. It was really interesting the logic you implemented.
  • βœ… The animation you added when they're matched was an excellent addon. 🐱

Required Changes ♻️

Check the comments under the review.

Optional suggestions

Every comment with the [OPTIONAL] prefix is not crucial enough to stop the approval of this PR. However, I strongly recommend you to take them into account as they can make your code better.

Cheers and Happy coding!πŸ‘πŸ‘πŸ‘


This is a feature teammate review. Remember that everything said here is intended for both of us to learn and grow as Software Developers πŸ’» and we may have different approaches or perspectives. That being said, don't hesitate to comment if you disagree with one required change. We can talk everything through 🐱

@JohnFTitor JohnFTitor added the enhancement New feature or request label Feb 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Code them all
Awaiting triage
Development

Successfully merging this pull request may close these issues.

[8pt] Implement A new game Page
2 participants