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

add star rating play #513

Merged
merged 10 commits into from
Sep 2, 2022
Merged

add star rating play #513

merged 10 commits into from
Sep 2, 2022

Conversation

frankiefab100
Copy link
Contributor

@frankiefab100 frankiefab100 commented Aug 20, 2022

This fixes issue #464

First thing, PLEASE READ THIS: ReactPlay Code Review Checklist

Description

A Star rating play shows the use of useState hook

Fixes #464

Type of change

Please delete options that are not relevant.

  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Locally on localhost

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 20, 2022

@frankiefab100 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 21, 2022

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

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

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 early impression

  1. Unwanted vertical and horizontal; scrollbar appearing
  2. It would be great if you can associate some titles with each start. something like "Bad", "Not OK", "Good", "Very Good", "Spelbound"
  3. it would be even better if you can give me some options to choose from. Like you have already implemented with star, probably another option would be smilies and so on

No 1 is mandatory to fix whereas No 2 and No 3, I would leave with you.
Once done I can start reviewing.

@frankiefab100
Copy link
Contributor Author

Few early impression

  1. Unwanted vertical and horizontal; scrollbar appearing
  2. It would be great if you can associate some titles with each start. something like "Bad", "Not OK", "Good", "Very Good", "Spelbound"
  3. it would be even better if you can give me some options to choose from. Like you have already implemented with star, probably another option would be smilies and so on

No 1 is mandatory to fix whereas No 2 and No 3, I would leave with you. Once done I can start reviewing.

Alright

@koustov
Copy link
Member

koustov commented Aug 25, 2022

Hey @frankiefab100 , let us know when its ready

@frankiefab100
Copy link
Contributor Author

frankiefab100 commented Aug 27, 2022

Hey @frankiefab100 , let us know when its ready

Hi @koustov I have made the necessary changes

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 dropped

src/plays/star-rating/Readme.md Outdated Show resolved Hide resolved
src/plays/star-rating/Readme.md Outdated Show resolved Hide resolved
src/plays/star-rating/StarRating.js Outdated Show resolved Hide resolved
src/plays/star-rating/StarRating.js Outdated Show resolved Hide resolved
src/plays/star-rating/StarRating.js Show resolved Hide resolved
src/plays/star-rating/StarRating.js Show resolved Hide resolved
src/plays/star-rating/styles.css Outdated Show resolved Hide resolved
@atapas
Copy link
Member

atapas commented Sep 1, 2022

@frankiefab100 let's close this one soon and get in.

@frankiefab100
Copy link
Contributor Author

@frankiefab100 let's close this one soon and get in.

I pushed new commits yesterday. Waiting for a review

@atapas
Copy link
Member

atapas commented Sep 1, 2022

@frankiefab100 let's close this one soon and get in.

I pushed new commits yesterday. Waiting for a review

Ok @koustov will close it then tonight.

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.

Final comment. Once implemented, its ready to merge

Comment on lines 8 to 9
width: 100vw;
height: 100vh;
Copy link
Member

Choose a reason for hiding this comment

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

100vh and 100vw are creating scroll to the container.
Replace the height and width with

height: 100%
width: 100%

It will solve the scroll issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, thanks.

I wonder why it looks okay from my end.

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.

Only comment
image

Fabulous job @frankiefab100

@koustov koustov merged commit 456c79c into reactplay:main Sep 2, 2022
@frankiefab100
Copy link
Contributor Author

Only comment image

Fabulous job @frankiefab100

Thank you 😄

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.

[Add a Play]: Star rating
3 participants