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

Fix #2534 - Add clearable ratings #2541

Merged
merged 3 commits into from
Sep 15, 2023

Conversation

Grygon
Copy link
Contributor

@Grygon Grygon commented Sep 9, 2023

What type of PR is this?

  • bug

What this PR does / why we need it:

Allow clearing ratings

Which issue(s) this PR fixes:

Fixes #2534

Testing

Added rating, refreshed to validate. Cleared rating, refreshed to validate.

Release Notes

Ratings can now be removed by tapping the already-selected rating a 2nd time.

@michael-genson
Copy link
Collaborator

michael-genson commented Sep 9, 2023

Looks good! The only thing I'm not huge on is that it changes the value to "0" rather than null/undefined, which might be confusing (we'll have some recipes with a null value and some with a zero value, even though they mean the same thing). It also could be an issue if we ever decide to implement a true 0 rating.

Can you modify the updateRating function to convert 0 to undefined? Something like:

function updateRating(val: number) {
  if (val === 0) {
    val = undefined;
  }
  ...
}

I'm not 100% sure the backend will actually update it to undefined/null so we might need to tweak the backend logic

Copy link
Collaborator

@hay-kot hay-kot left a comment

Choose a reason for hiding this comment

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

See @michael-genson commnets, thanks for the PR definitely will have some happy users with this one!

@Grygon
Copy link
Contributor Author

Grygon commented Sep 15, 2023

Looking into it! It does look like the CRUD API doesn't like accepting it to be undefined.

From some testing it'll accept null, so I'll swap out the types for ratings to make it number | undefined | null and use that!

Copy link
Collaborator

@michael-genson michael-genson 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, thanks! Just a small change with the type definition

frontend/lib/api/types/recipe.ts Show resolved Hide resolved
@hay-kot hay-kot enabled auto-merge (squash) September 15, 2023 19:06
@hay-kot hay-kot self-requested a review September 15, 2023 19:06
@hay-kot hay-kot merged commit 9a04b11 into mealie-recipes:mealie-next Sep 15, 2023
@Grygon Grygon deleted the Grygon/fix-2534 branch September 15, 2023 19:19
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.

[BUG] - Star Rating cant remove all stars
3 participants