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

a Simple and Fun Quiz App #170

Merged
merged 6 commits into from
May 22, 2022
Merged

a Simple and Fun Quiz App #170

merged 6 commits into from
May 22, 2022

Conversation

Angryman18
Copy link
Member

Description

Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context. List any dependencies that are required for this change.

Fixes # (issue)

Type of change

Please delete options that are not relevant.

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce.

Checklist:

  • I have performed a self-review of my own code
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works

@vercel
Copy link

vercel bot commented May 14, 2022

Someone is attempting to deploy a commit to a Personal Account owned by @atapas on Vercel.

@atapas first needs to authorize it.

@Angryman18
Copy link
Member Author

Its really a dirty work and dirty code also.

@vercel
Copy link

vercel bot commented May 14, 2022

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

Name Status Preview Updated
react-play ✅ Ready (Inspect) Visit Preview May 21, 2022 at 6:09AM (UTC)

Copy link
Contributor

Choose a reason for hiding this comment

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

Very Amazing

@atapas
Copy link
Member

atapas commented May 14, 2022

This is one of the most incredible play I've seen! I shall start doing code reviews soon.

@Programming-School-Pro-Coding
Copy link
Contributor

This is one of the most incredible play I've seen! I shall start doing code reviews soon.

Give me the reviewing and merging i am free

@Angryman18
Copy link
Member Author

This is one of the most incredible play I've seen! I shall start doing code reviews soon.

Thanks. I see a bug. Will fix at next push.

@Angryman18
Copy link
Member Author

sorry i was mistaken about the bug. i think its working good. although i fixed a "key" was missing while mapping array.

@atapas
Copy link
Member

atapas commented May 16, 2022

@Angryman18 Could you please create an issue for the play and associate with the PR to start the review?

@Angryman18
Copy link
Member Author

@Angryman18 Could you please create an issue for the play and associate with the PR to start the review?

Created sir.

@atapas atapas linked an issue May 16, 2022 that may be closed by this pull request
@atapas
Copy link
Member

atapas commented May 16, 2022

@Angryman18 Could you please create an issue for the play and associate with the PR to start the review?

Created sir.

Thanks but it was not done correctly.

So next time,

  • You need to create a play request if it is a play, else a bug or feature.
  • After creating the issue you can attach it to a PR(if already exist) from the right side setting of an issue.

@Angryman18
Copy link
Member Author

ah. sorry about that. not very good on github yet but i learned a lot in recent days.

@atapas atapas self-requested a review May 16, 2022 04:52
@atapas
Copy link
Member

atapas commented May 16, 2022

This is one of the most incredible play I've seen! I shall start doing code reviews soon.

Give me the reviewing and merging i am free

Please go ahead and finish the review

@atapas atapas requested a review from koustov May 16, 2022 10:54
Copy link
Contributor

Choose a reason for hiding this comment

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

LGTM

@atapas
Copy link
Member

atapas commented May 16, 2022

LGTM

Thanks, @koustov please finish your review too.

@koustov
Copy link
Member

koustov commented May 17, 2022

Started reviewing. I will finish it soon.

src/meta/play-meta.js Outdated Show resolved Hide resolved
src/plays/fun-quiz/EndScreen.jsx Outdated Show resolved Hide resolved
src/plays/fun-quiz/EndScreen.jsx Show resolved Hide resolved
src/plays/fun-quiz/EndScreen.jsx Show resolved Hide resolved
src/plays/fun-quiz/EndScreen.jsx Outdated Show resolved Hide resolved
src/plays/fun-quiz/QuizScreen.scss Show resolved Hide resolved
src/plays/fun-quiz/QuizScreen.scss Outdated Show resolved Hide resolved
src/plays/fun-quiz/QuizScreen.scss Outdated Show resolved Hide resolved
src/plays/fun-quiz/QuizScreen.scss Outdated Show resolved Hide resolved
src/plays/fun-quiz/QuizScreen.scss Outdated Show resolved Hide resolved
@koustov
Copy link
Member

koustov commented May 18, 2022

@Angryman18 this is a great play ❤. I am fully energized to see the granularity maintained here.

@Angryman18
Copy link
Member Author

ok i will fix by night

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.

some questions added. also please notify once you are done with all comments.

src/plays/fun-quiz/FrontScreen.jsx Outdated Show resolved Hide resolved
src/plays/fun-quiz/QuizScreen.scss Outdated Show resolved Hide resolved
src/plays/fun-quiz/EndScreen.jsx Show resolved Hide resolved
src/plays/fun-quiz/EndScreen.jsx Outdated Show resolved Hide resolved
@atapas
Copy link
Member

atapas commented May 21, 2022

i dont see any issue with hover. we only want to show pointer when its hovered. so eitherway it works the same. and i believe only questions would be coming from the api so i didnt add additional validiation there. and for nullable, i dont understand if you are talking about current question if so then current question cannot be null. its defined with a value and some certain conditions are applied for updating it's value (when timer gets timed out or user clicks "confirm" button or at veryfirst clicking "Let start" button).

Look like we are close to getting done with the comments. Can't wait to see this one crossing the border...

@Angryman18 @koustov

@koustov
Copy link
Member

koustov commented May 21, 2022

@Angryman18 is it ready to be reviewed one last time?

Note: Please reply to every comment. In fact, if you disagree with the suggestion given, just respond with reasoning. I want to resolve all conversations before approving any PR, this will be really helpful for any future tracking.

@Angryman18
Copy link
Member Author

i think the required changes are made.

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

@koustov koustov merged commit d0559f1 into reactplay:main May 22, 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.

[Play]: a Simple and Fun Quiz App #170
4 participants