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

feat: sort plays by Newest,Oldest,Most Liked,Random #1235

Merged
merged 7 commits into from
Sep 20, 2023
Merged

feat: sort plays by Newest,Oldest,Most Liked,Random #1235

merged 7 commits into from
Sep 20, 2023

Conversation

sudipkundu999
Copy link
Contributor

First thing, PLEASE READ THIS: ReactPlay Code Review Checklist

Description

Currently, the plays are arranged in the date-time sorted order. The recent play comes in the front.

  • Adding Sort by:
    • Newest
    • Oldest
    • Most Liked
    • Random
  • Saving this preference to localStorage

Fixes #1159

Type of change

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

How Has This Been Tested?

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

  • Go to /plays
  • You'll see a Sort by dropdown between the Banner and Play list. By default Newest is selected.
  • Change the dropdown to Oldest,Most Liked, Random to see the plays sort by accordingly.

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

Screenshots or example output

image

@vercel
Copy link

vercel bot commented Sep 7, 2023

@sudipkundu999 is attempting to deploy a commit to a Personal Account owned by @reactplay on Vercel.

@reactplay first needs to authorize it.

@netlify
Copy link

netlify bot commented Sep 7, 2023

Deploy Preview for reactplayio ready!

Name Link
🔨 Latest commit f291eac
🔍 Latest deploy log https://app.netlify.com/sites/reactplayio/deploys/65002f451bc6bb0008a56289
😎 Deploy Preview https://deploy-preview-1235--reactplayio.netlify.app/
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@Angryman18
Copy link
Member

is this PR ready for Review ? @sudipkundu999

@Angryman18 Angryman18 requested review from Angryman18 and removed request for Angryman18 September 8, 2023 03:52
@sudipkundu999
Copy link
Contributor Author

Yes, this PR is ready for review @Angryman18

@sudipkundu999
Copy link
Contributor Author

@Angryman18 @atapas Please review

@atapas
Copy link
Member

atapas commented Sep 10, 2023

@sudipkundu999 build fails.. please fix the lints locally first

image

@sudipkundu999
Copy link
Contributor Author

@atapas I ran yarn lint and yarn format locally.
I'm not getting any error, these are 2 existing warnings. What step am I missing here?
image

@MrTea7171
Copy link
Contributor

@atapas I ran yarn lint and yarn format locally. I'm not getting any error, these are 2 existing warnings. What step am I missing here? image

you need to fix those warning that is the lint error we are talking about

@sudipkundu999
Copy link
Contributor Author

@atapas I ran yarn lint and yarn format locally. I'm not getting any error, these are 2 existing warnings. What step am I missing here? image

you need to fix those warning that is the lint error we are talking about

Hey, but I'm not seeing the error Callbacks must be listed after all other props when I run yarn lint
this is the error that is failing the build. How can I see this error locally?

Other warnings were from before. That shouldn't be the reason for build failing

@sudipkundu999
Copy link
Contributor Author

@atapas I ran yarn lint and yarn format locally. I'm not getting any error, these are 2 existing warnings. What step am I missing here? image

Also in this screenshot yarn lint throws a warning about the typescript version 5.2.2 which is the latest one and is unsupported by @typescript-eslint/typescript-estree

Shouldn't we freeze the typescript version instead of always picking the latest version?
In package.json we have "typescript": "*"

@atapas
Copy link
Member

atapas commented Sep 11, 2023

@sudipkundu999 please align this right..

image

@sudipkundu999
Copy link
Contributor Author

@sudipkundu999 please align this right..

image

Done! This is how it looks!
image

Copy link
Member

@Angryman18 Angryman18 left a comment

Choose a reason for hiding this comment

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

Approved with some comments

@atapas
Copy link
Member

atapas commented Sep 18, 2023

@supminn Do you have further comments on this?

@supminn
Copy link
Contributor

supminn commented Sep 20, 2023

@supminn Do you have further comments on this?

no, It looks good.

Copy link
Member

@priyankarpal priyankarpal left a comment

Choose a reason for hiding this comment

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

Looks Good 🎉

@atapas atapas merged commit 883c114 into reactplay:main Sep 20, 2023
4 of 5 checks passed
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.

✨ [Feature request]: Randomize play cards
6 participants