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

Add custom scaling option #1345

Merged
merged 14 commits into from
Jun 9, 2022
Merged

Add custom scaling option #1345

merged 14 commits into from
Jun 9, 2022

Conversation

Bentipa
Copy link
Contributor

@Bentipa Bentipa commented Jun 5, 2022

This implements the possibility to enter a custom scale factor.

When no servings are configured and no scale set, it looks like this:
image
When clicking on x1 (a tooltip shows the option "Edit Scale"), the scale dialog is shown:
image

The user can then enter any number (including 0.5 for example) which then after clicking save changes the scale.
The scale can also be reset to 1 using the IconButton.
image

When servings are configured (in this example servings=1) it looks default like:
image

When the scale is modified it looks like this:
image

Some open questions might be:

  • When servings <= 1 the word "servings" would need to be singular (not that important in my opinion)
  • I modified the default visualization of the servings amount to make it more clear what happens (if there is a scaling happening and what the final servings are)
  • In the "cook"-screen, the scale is not taken from the previous page (?) we should maybe also add the dialog here when clicking on scale

In addition to the custom button now, I also added tooltips for increase and decrease scale to make it clearer what the buttons do.
I am happy to get some feedback about the implementation :)

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.

Very Cool! Thanks for taking a look at this, overall I like the implementation. I would like this to be implemented with a Menu type interface instead of a dialog. Like how the "Image" and "Setting" buttons have. I think that would be easier to use.

Example

CleanShot 2022-06-05 at 10 59 52

Let me know if you want to take a stab at implementing with the other style, if not I can take this one over the finish line.

Thanks! 🚀

frontend/pages/recipe/_slug/index.vue Outdated Show resolved Hide resolved
frontend/pages/recipe/_slug/index.vue Outdated Show resolved Hide resolved
@Bentipa
Copy link
Contributor Author

Bentipa commented Jun 6, 2022

Ah yeah I originally wanted to use exactly such solution but did not know how to implement it.
I will take a look and try to use this component instead :)

@Bentipa
Copy link
Contributor Author

Bentipa commented Jun 6, 2022

image
image

Should now be as you wanted :)

@Bentipa Bentipa requested a review from hay-kot June 8, 2022 19:10
@hay-kot
Copy link
Collaborator

hay-kot commented Jun 8, 2022

Thanks for your work on this one. Couple changes I made

  1. Fixed the formatting using the linter.
  2. Made some minor UI changes

CleanShot 2022-06-08 at 11 29 22@2x

CleanShot 2022-06-08 at 11 29 59@2x

I'm not sure I like how the scaling is displayed, I'll go ahead and merge it as is, but I might make some changes to the display later down the line depending on feedback.

@Bentipa
Copy link
Contributor Author

Bentipa commented Jun 8, 2022

Ah looks nice that way yeah.

Do you mean the scale display in the field (e.g., 4 x 5 = 20 servings?)

@hay-kot
Copy link
Collaborator

hay-kot commented Jun 8, 2022

Do you mean the scale display in the field (e.g., 4 x 5 = 20 servings?)

Yeah, it causes a significant UI shift when you tap the -/+ button from 1 to 2 which isn't ideal, but maybe other people will like the clear display 🤷

@Bentipa
Copy link
Contributor Author

Bentipa commented Jun 8, 2022

We could move + / - to the left maybe? Then it would not move
Also do you use any special formatter for the files? Mine changed too much other stuff

(this might also make more sense as the scale is "before" the servings)

@hay-kot
Copy link
Collaborator

hay-kot commented Jun 8, 2022

Prettier and VS Code does all the formatting for me. Depending on your editor it should know to use the prettier config in the project.

What about a caption text?

CleanShot 2022-06-08 at 12 08 02@2x

@Bentipa
Copy link
Contributor Author

Bentipa commented Jun 8, 2022

Hm okay I got both of them, I will check again.

If we now have enough space, we could explain it maybe?
5 (Scale) x 10 (Basic Recipe Servings) = 50 Servings

Or is this too detailed?

@Bentipa
Copy link
Contributor Author

Bentipa commented Jun 8, 2022

Or maybe we should think about just defining servings and make the scale something internal?
That might make the most sense now as I think about it

(at least when your recipe has a servings number defined, else an ingredient scale makes sense)

@hay-kot hay-kot merged commit 8836a25 into mealie-recipes:mealie-next Jun 9, 2022
@Bentipa
Copy link
Contributor Author

Bentipa commented Jun 9, 2022

Okay so you just removed the text and merged now without further discussion? 😅 Sorry but this is confusing for me. That's not how I interpret contribution..

@hay-kot
Copy link
Collaborator

hay-kot commented Jun 9, 2022

Okay so you just removed the text and merged now without further discussion? 😅 Sorry but this is confusing for me. That's not how I interpret contribution..

I had a response typed up but forgot to submit it before I merged. I felt like there were two things in this one, and I wanted to get that initial feature merged so I could make some other some-what related changes without causing too many merge conflicts.

I removed the text and merged the core feature while keeping the main UI the same as it was. What I was going to ask was for you to submit a different PR with a proposed update for how servings are displayed and then we can talk about that in isolated and hopefully get some feedback from the community.

Hope that's fair? Sorry about the confusion.

@Bentipa
Copy link
Contributor Author

Bentipa commented Jun 9, 2022

Ahh okay now it makes much more sense :D

I'll make a separate PR about the display in the next days.

What do you think about switching from servings to scaled input when there is a base servings amount given?

Yeah sounds good, thanks for the quick explanation!

@zierbeek
Copy link
Contributor

Hi there! thank you for your work on the custom scaling. I feel that it would be beter to be able to change the number of servings. is that something which was considered or will be considered in the future?

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.

3 participants