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: Ratings UI and Filter Issues #3459

Merged

Conversation

michael-genson
Copy link
Collaborator

What type of PR is this?

(REQUIRED)

  • bug

What this PR does / why we need it:

(REQUIRED)

This PR fixes two issues with the new user ratings:

RecipeRating Component

There are various quirks with setting ratings when the group rating matches the desired user rating For instance, if the group rating is 4, and you want to set your user rating to 4, the UI interprets this as you setting it to 0.

Now we use two v-rating components to achieve the same effect, which greatly simplifies the logic (and removes the same-rating edge case).

Sort Recipes By Rating

The rating sort on the home page can be jank depending on database-specific handling (e.g. I think postgres defaults to "nulls first" and sqlite defaults to "nulls last", or something like that) so I made the null handling explicitly nulls last (unless overridden with a query param).

I also coalesced 0 values to null, since currently they are equivalent (you can't rate something 0, you can only remove your rating).

Which issue(s) this PR fixes:

(REQUIRED)

N/A

Testing

(fill-in or delete this section)

Manual frontend testing

@Kuchenpirat Kuchenpirat self-assigned this Apr 19, 2024
Copy link
Collaborator

@Kuchenpirat Kuchenpirat left a comment

Choose a reason for hiding this comment

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

Looks all good.
This seems way less convoluted than the way we used to do it before 👍

@Kuchenpirat Kuchenpirat merged commit 711dd93 into mealie-recipes:mealie-next Apr 19, 2024
10 checks passed
@michael-genson
Copy link
Collaborator Author

Yeah my original implementation sucked, haha

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

2 participants