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

Pokemon stats #534

Merged
merged 13 commits into from Sep 5, 2022
Merged

Pokemon stats #534

merged 13 commits into from Sep 5, 2022

Conversation

yung-coder
Copy link
Contributor

First thing, PLEASE READ THIS: ReactPlay Code Review Checklist

Description

So here I have tried to build a pokemon stats app that will give you information about a particular pokemon on search.

Fixes # (issue)
#527

Type of change

  • [ ✔️] New feature (non-breaking change which adds functionality)

How Has This Been Tested?

it has been tested by me in my local system for all the cases.

Checklist:

  • [✔️ ] I have performed a self-review of my own code
  • [✔️ ] I have commented my code, particularly in hard-to-understand areas
  • [✔️ ] I have made corresponding changes to the documentation
  • [✔️ ] My changes generate no new warnings
  • [✔️ ] I have added tests that prove my fix is effective or that my feature works
  • [✔️ ] New and existing unit tests pass locally with my changes
  • [✔️ ] Any dependent changes have been merged and published in downstream modules

@vercel
Copy link

vercel bot commented Aug 31, 2022

@yung-coder is attempting to deploy a commit to a Personal Account owned by @reactplay on Vercel.

@reactplay first needs to authorize it.

@vercel
Copy link

vercel bot commented Aug 31, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
react-play ✅ Ready (Inspect) Visit Preview Sep 4, 2022 at 3:28AM (UTC)

Copy link
Member

@atapas atapas left a comment

Choose a reason for hiding this comment

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

A few initial comments

src/plays/pokemon-stats/PokemonStats.js Outdated Show resolved Hide resolved
src/plays/pokemon-stats/components/Pokemoncard.jsx Outdated Show resolved Hide resolved
@atapas
Copy link
Member

atapas commented Sep 1, 2022

@yung-coder

is it under development? I have given a few comments. Apart from it, I am getting some errors in the console too..The search doesn't work for me

image

@yung-coder
Copy link
Contributor Author

Write the name of pokemon in small letters it will work...

@atapas

@atapas
Copy link
Member

atapas commented Sep 1, 2022

Visit Preview

I see :)

Then you fix that to lower the name always before passing to the API.. Let user type in case insensitive way.

Also, a couple of things,

  • You may want to handle the error case when something doesn't;t have info or invalid
  • The Pokemon description card... the info looks as clickable.

@yung-coder
Copy link
Contributor Author

Okay i will handle it as case sensitive and for card description the info is clickable for details should i do some specific changes....?

@atapas
Copy link
Member

atapas commented Sep 1, 2022

Okay i will handle it as case sensitive and for card description the info is clickable for details should i do some specific changes....?

Are they really meant to be clickable? Because when I click on them, nothing happens. So if they are not supposed to be clickable, change the cursor stye to none.

@yung-coder
Copy link
Contributor Author

Yeah nothing will happen it will just change the color on hover ...

@atapas
Copy link
Member

atapas commented Sep 1, 2022

Yeah nothing will happen it will just change the color on hover ...

OK

@yung-coder
Copy link
Contributor Author

So, should i remove cursor pointer or not...

@atapas
Copy link
Member

atapas commented Sep 1, 2022

So, should i remove cursor pointer or not...

Yes Yes Yes :)

@yung-coder
Copy link
Contributor Author

Hey, @atapas I have made the changes pls give it a look .....

Copy link
Member

@koustov koustov left a comment

Choose a reason for hiding this comment

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

I haven't jumped into code however here are few observations.

  1. If do not type anything and hit search, I get
    image
  2. I am not a pokemon follower so I really don't know any name other than Pikachu. So cant type. Shall we have some drop-down or type ahead for exploring?
  3. When I type, upon hitting enter should search the text. I mean, that should be a submit button.
  4. Once a stat is shown, how do I search for another one?
  5. In the mobile resolution, I am feeling like losing padding (iPhone SE)

@yung-coder
Copy link
Contributor Author

For button type sumbit i will do it and for the feature of drop-down surely it's a good idea but i don't have any idea how can we implement it so if you have an idea please let me know and for resolutions i will look at it and if we once search a pokemon name we will be shown it's stats like hp and etc....if we want to search for another we have to go back and then we can search......

@koustov

@yung-coder yung-coder requested review from koustov and atapas and removed request for atapas and koustov September 2, 2022 19:48
@yung-coder
Copy link
Contributor Author

Hey, @atapas and @koustov please give it a look I have made the changes I think it's good but let me know if you want any modifications ...

package.json Show resolved Hide resolved
src/plays/pokemon-stats/PokemonStats.js Outdated Show resolved Hide resolved
Copy link
Member

@koustov koustov left a comment

Choose a reason for hiding this comment

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

Few comments

src/plays/pokemon-stats/PokemonStats.js Outdated Show resolved Hide resolved
src/plays/pokemon-stats/PokemonStats.js Outdated Show resolved Hide resolved
src/plays/pokemon-stats/Readme.md Show resolved Hide resolved
src/plays/pokemon-stats/Readme.md Outdated Show resolved Hide resolved
src/plays/pokemon-stats/Readme.md Outdated Show resolved Hide resolved
src/plays/pokemon-stats/components/Search.jsx Show resolved Hide resolved
src/plays/pokemon-stats/PokemonStats.js Show resolved Hide resolved
src/plays/pokemon-stats/components/search.css Outdated Show resolved Hide resolved
src/plays/pokemon-stats/components/search.css Outdated Show resolved Hide resolved
src/plays/pokemon-stats/components/search.css Outdated Show resolved Hide resolved
@koustov
Copy link
Member

koustov commented Sep 3, 2022

For button type sumbit i will do it and for the feature of drop-down surely it's a good idea but i don't have any idea how can we implement it so if you have an idea please let me know and for resolutions i will look at it and if we once search a pokemon name we will be shown it's stats like hp and etc....if we want to search for another we have to go back and then we can search......

@koustov

  • Drop down: Even if you do not get any API to list all pokemon, you can hard code a small list. You can use MUI auto-complete.
  • Search again: I feel the search should be independent from the result. I mean, we can search continuously and get the result instead of refreshing the page or go back and forth.

@yung-coder
Copy link
Contributor Author

Mui auto complete i was not knowing about it i will give it a look and try to implement...

And for the refreshing yeah we can have search and result together but i thought this card design would look cool if you want to change result display section please let me know...

@koustov

@koustov
Copy link
Member

koustov commented Sep 3, 2022

Mui auto complete i was not knowing about it i will give it a look and try to implement...

And for the refreshing yeah we can have search and result together but i thought this card design would look cool if you want to change result display section please let me know...

@koustov

And you are right, the card design is definitely cool and I liked the glassy effect. However, having search and the card separately wouldn't make any impact on the glamour, and that's what I feel

@yung-coder
Copy link
Contributor Author

Changes done @koustov @atapas

Copy link
Member

@koustov koustov left a comment

Choose a reason for hiding this comment

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

lgtm

@yung-coder
Copy link
Contributor Author

Hey @atapas is it all good ...?

@atapas
Copy link
Member

atapas commented Sep 5, 2022

@yung-coder Some conversations are opened. Could you please respond to them and close?

@yung-coder
Copy link
Contributor Author

Done ✔️ @atapas

@atapas atapas merged commit 98b02c4 into reactplay:main Sep 5, 2022
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.

None yet

3 participants