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: refactor recipe scaling #4298

Merged
merged 4 commits into from
Oct 1, 2024
Merged

Conversation

Kuchenpirat
Copy link
Collaborator

@Kuchenpirat Kuchenpirat commented Oct 1, 2024

What type of PR is this?

  • feature

What this PR does / why we need it:

This PR refactors the recipe scaler for a smoother user experience.

Previous Behavior:

The old scaler was based on a base scale factor of 1, which would autmatically double/half the recipe using the + | - Buttons. However, to scale a recipe from 4 servings to 6, for example, users had to manually calculate the scale (e.g., entering 1.5). This process could be cumbersome, especially when the math wasn't straightforward.

New Behavior:

With this update, the scale is inferred directly from the current serving size, allowing users to adjust servings more intuitively by using the + | - buttons which will now increase/decrease the serving size by one. Additionally, the manual input field now allows users to directly enter the desired number of servings, eliminating the need to calculate a scale value.

This update also removes the restriction that required a serving count for the component to be rendered. Now, the component will default to a serving size of 1 if no serving count is provided.

If the serving size cannot be infered from the servings count string (eg. six servings) the scaler will default to a value of one. If the serving size is increased it will prepend the Yield string with <number>x resulting in e.g. "2x six servings" which is ugly but is the best we can do i think.

An alternative implementation was discussed in #3755, where scaling below the original serving size would be handled by halving. However, this current implementation supports fractional servings (e.g., entering 0.5 manually), which should cover most use cases. For large numbers one can also directly set the number of serving sizes so clicking + should not be required.

Which issue(s) this PR fixes:

Discussions:

Special notes for your reviewer:

  • Removed the note added in docs: add note on recipe scaling to docs #4287 as it is no longer applicable.
  • The findMatch function from the use-extract-recipe-yield composable was used. We could alternatively return this number automatically with with the useExtractRecipeYield function.
  • This change may affect recipes with fractional serving sizes (e.g., 1/2 baking tray). Previously, the scaler increased in increments like 0.5 -> 1 -> 1.5, but now it increases as 0.5 -> 1.5 -> 2.5. I feel like this should not concern the majority of recipes.

Testing

Manual testing, including fractional serving sizes.

@Kuchenpirat
Copy link
Collaborator Author

@bartkummel if you have any thought on this feel free to share.
Always valuable to have somebody with a different view contribute 👍

@bartkummel
Copy link
Contributor

Generally speaking, I think this is a better solution. However, I’m not sure if 1 is the best default value. In my experience most recipes serve 4.

@Kuchenpirat
Copy link
Collaborator Author

That might make sense, my thought process for going with 1 was that if we scale up from one and i want to comunicate that back to the user the scaler would show something like "3/4x four muffins" which looks even more complicated than just "2x four muffins"

Happy to change this tho, if it is clear that this would be prefered.

@bartkummel
Copy link
Contributor

Or we should add a global setting for default serving size

@Kuchenpirat
Copy link
Collaborator Author

The default serving size is just the fallback so i don't think that makes too much sense since recipes will naturally have different serving sizes and my guess is that the people who do not fill out the serving sizes will not bother to recalculate the recipe to a common basis.
This will make this unreliable.

AND some people will 100% think that this will recalculate their recipes with serving sizes to the default serving size, which might actually be an interesting idea.

michael-genson
michael-genson previously approved these changes Oct 1, 2024
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.

This looks good to me. I see no issue with the default serving size being 1, it's the closest we're going to get to something that makes sense (if no serving is specified, it's "1x the current recipe").

In the future we can look into improving the findMatch method to account for words, etc, or maybe even attempt to parse it server-side.


Approved, though I left one comment. May also want to keep this one open for discussion.

@Kuchenpirat
Copy link
Collaborator Author

In the future we can look into improving the findMatch method to account for words, etc, or maybe even attempt to parse it server-side.

i think having this eventually in a backend db might make sense. We don't do much with that info currently, but trying to parse it on creation and then have separate entries for amount and text makes sense to me.
When the parsing failed the user can input a number (followed by text) which we know to be a number and don't have to find in some string and save that to the backend

@Kuchenpirat Kuchenpirat merged commit 14dbd79 into mealie-next Oct 1, 2024
13 checks passed
@Kuchenpirat Kuchenpirat deleted the feat--refactor-recipe-scaling branch October 1, 2024 14:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[v1.0.0b] [Task] - Recipe scaling UX
3 participants