-
-
Notifications
You must be signed in to change notification settings - Fork 763
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: Structured Yields #4489
feat: Structured Yields #4489
Conversation
Thanks @Kuchenpirat! Love the feedback. I've gone ahead and implemented these suggestions. Refactoring the recipe info at the top was long overdue, and I really needed to do it to get the yield in there, so that's part of this PR now too. Happy to take feedback on the new layout. This is the new recipe info layout, including the yield (if set): The yield will no longer scale, since it's in the info card. I don't think changing this value with scaling makes sense, because it's no longer visually anywhere near the scale controls. The time cards on the right are always stacked, and always in their own part of the info card. If there's not enough room it wraps: With the refactor, we don't have a bunch of duplicated elements when in/out of landscape. I also separated the info card display from the editor. This led to some... weirdness in the editor with the layout, so I added some text to the top of the editor: This also resulted in some tweaks to landscape mode: Additionally, scale now works as you suggested: we look for servings qty first, then yield qty, then a default of "1". The text always reads "Serves X" regardless of which value it uses (I think this makes sense since you're scaling how much you're making, however regardless changing this would be a nightmare). |
frontend/components/Domain/Recipe/RecipePage/RecipePageParts/RecipePageHeader.vue
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happy to hear my feedback was well recieved and i think we are heading towards the right direction, but i feel a few things have not yet clicked into their right place.
So onto my next round of Feedback, i have skimmed over the backend and it seems pretty solid (as usual on a michael PR 👍) but i have a few more notes on the Frontend UI/UX
frontend/components/Domain/Recipe/RecipePage/RecipePageParts/RecipePageInfoCard.vue
Show resolved
Hide resolved
frontend/components/Domain/Recipe/RecipePage/RecipePageParts/RecipePageInfoCard.vue
Outdated
Show resolved
Hide resolved
frontend/components/Domain/Recipe/RecipePage/RecipePageParts/RecipePageInfoCard.vue
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, i agree lets push the RecipeInfo redesign into a separate PR.
There are probably a lot of things that can be done there (e.g. the two separate buttons for last made and "I made this", all time elements having the same icon ...)
For the main part of the PR this is all working as expected and i couldn't find a way to break it 😊🚀
There is one super small suggestion on the edit button. Feel free to merge if you think this is not necessairy.
The next logical step for me to make this even more powerfull out of the box would be to parse the ingredient quantities on recipe creation. This would enable full scaling capabilities even for non parsed recipes 🤔
That's not a bad idea. While the parser isn't the most consistent, it's much better at the quantity portion |
Yeah, i just thought, what we are doing here could work for ingredient quantities, which is really the only thing needed for scaling. The actual ingredient can stay a note. |
What type of PR is this?
(REQUIRED)
What this PR does / why we need it:
(REQUIRED)
This PR adds a new column on the recipe
recipe_yield_quantity
. This splits the recipe's yield into two fields: qty and description/unit (where the old column ofrecipe_yield
serves as the latter). We also attempt to parse existing yields upon migration.For parsing, we assume the yield string starts with the quantity, and the rest of it is the unit. We also only consider numerical quantities (so we aren't parsing "One Serving" into "1" and "Serving"). Users can manually update the quantity/unit themselves, so it doesn't have to be perfect. Backwards compatibility is maintained since both the quantity and the unit are displayed, although there are a few edge-cases where it's not perfect (e.g. "3-4 servings" will display as "3 -4 servings", because we parse "3" as the qty, and "-4 servings" as the unit). I think it's preferred to parse these edge-cases and expect the extra space, rather than not parse them, since it populated the yield qty (which we'll leverage in future PRs).
For scaling, the user experience hasn't changed much, but I had to re-work how we do it since we don't extract yields anymore. Instead scaling requires that the recipe has a yield quantity. The only user-facing change is that previously we allowed scaling even if we couldn't parse the servings string (by assuming the quantity is 1). Since it's user-editable now I don't see why we need to support this anymore (if you want it to work, set the quantity).
For editing it's a simple UI change:
When displayed it looks the same as before:
While this PR doesn't add a ton of value upfront, it opens the door to a lot of enhancements (e.g. filtering by yield).
Which issue(s) this PR fixes:
(REQUIRED)
N/A
Testing
(fill-in or delete this section)
Frontend tests and pytest.