Skip to content

Creating a PokeCard component - #1

Open
erfemega wants to merge 2 commits into
mainfrom
csb-ijy6c
Open

Creating a PokeCard component#1
erfemega wants to merge 2 commits into
mainfrom
csb-ijy6c

Conversation

@erfemega

Copy link
Copy Markdown
Owner

No description provided.

Comment thread src/App.js Outdated
<div className="App">
<h1>Welcome to your second mini challenge!</h1>
<div className="pokemonsContainer">
<PokeCard pokemonData={mockPokemonData} />

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Since there are not much elements inside the mocked data, you could send them as properties for having the code more descriptive itself

Suggested change
<PokeCard pokemonData={mockPokemonData} />
const { name, sprites, video } = mockPokemonData;
<PokeCard sprites={sprites} name={name} video={video} />

Comment thread src/components/PokeCard.js Outdated
<img src={props.pokemonData.sprites.front_shiny} alt="Shiny" />
</div>
<div className="pokemonControls">
<a href={props.pokemonData.video} target="blank">

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Following what I said before, it makes the code cleaner here using less navigation through the pokemon obj.

Suggested change
<a href={props.pokemonData.video} target="blank">
PokeCard({ name, sprites, video })
<a href={video} target="_blank" rel="noreferrer">

I know it is not much visible with this case but you'll see it for sure in more complex components.

Comment thread src/styles.css
width: 100%;
}

.pokemonImages img {

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

nice 👌🏽 that you took care of the styling

Destructuring mockPokemonData and pass properties separated to the PokeCard component
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