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: Added a dedicated cookmode dialog that allows for individual scrolling #4464

Merged

Conversation

codetakki
Copy link
Contributor

@codetakki codetakki commented Oct 29, 2024

What type of PR is this?

(REQUIRED)

  • feature

What this PR does / why we need it:

(REQUIRED)

The cook mode currently only helps if we have linked ingredients to steps, this pr makes the cook mode useful even when no ingredients are linked. I have added a full-screen dialog that only has the ingredients and steps in a side by side view. Both lists are scrolled individual, resulting in both the info always being visible. I have taken insperation from ICA Sweden app, which has a similar view:
Screenshot_20241028_225043_ICA
Here is a example from mobile in landscape:
image

Which issue(s) this PR fixes:

(REQUIRED)

I have not created a issue, but had a discord conversation about something like this.

Special notes for your reviewer:

(fill-in or delete this section)

We could add additional things like timer to the dialog, making it more feature complete

Testing

(fill-in or delete this section)

I have tested the layout on mobile and desktop screen sizes, both giving an improved cooking view experience

@codetakki codetakki marked this pull request as ready for review October 29, 2024 08:36
@Kuchenpirat
Copy link
Collaborator

please update your title to start with feat:

@codetakki codetakki changed the title Added a dedicated cookmode dialog that allows for individual scrolling feat: Added a dedicated cookmode dialog that allows for individual scrolling Oct 29, 2024
@codetakki
Copy link
Contributor Author

For me it would be nice to lock the dialog to landscape mode, but im not sure how others feel about that

@boc-the-git
Copy link
Collaborator

For me it would be nice to lock the dialog to landscape mode, but im not sure how others feel about that

Definitely not, is how I feel about that :)

Can you please share screenshots of what this change looks like on a phone in portrait mode?

@codetakki
Copy link
Contributor Author

For me it would be nice to lock the dialog to landscape mode, but im not sure how others feel about that

Definitely not, is how I feel about that :)

Can you please share screenshots of what this change looks like on a phone in portrait mode?

In portait did not look great so i made it match the basic view:
image
It still has some function as it removes the image and other header things your not really interested in during cooking

@boc-the-git
Copy link
Collaborator

I'd probably need to see it in practice to understand the difference (comparing the same recipe side to side).
To me, that looks a significantly worse experience than what I see today in cook mode; though the current cook mode of course includes the requirement to add ingredients to steps.

🤔

@codetakki
Copy link
Contributor Author

codetakki commented Oct 29, 2024

It would also be a option to make it a separate feature, instead of replacing the existing cook mode, something like Focus mode or something idk.
Personally, i still think as i don't have to scroll up and down to see the product amounts.
Or the layout changes to something like this if we are on mobile and in landscape mode.
For clarity: if products are linked they are still shown under the steps as before, so if you are on portrait there is only the difference of hiding the header and options buttons. (still need to add timer)

@Kuchenpirat
Copy link
Collaborator

I like the direction you are going quite a lot and i feel the layout may might make even more sense than our current cookbook mode when in landscape mode.
Being able to check off ingredients you are done with, is visually much better than just hiding the step IMHO which is what we are currently doing in cookbook mode.

But there are a few things that could be improved in my opinion:

  • The UI is still a bit messy
    • X for closing out of cookbook mode is on the right side of the left column
    • Cook Mode symbol (which is also a toggle to disable cookmode) is still being shown when in cookmode which feels counterintuitive
  • I feel like hiding all other UI elements with the fullscreen Dialog is not the way to go, i would prefer to have still access to my appbar and being able to pop out the Nav Drawer
    • i often cook using my ipad so we are not always that space constrained when in cooking mode
  • If the user have linked ingredients to steps currently the ingredients will be shown twice (bellow the step and to the left).
    • I feel like there should be an advantage of doing that work.
      • something like having the ingredients scroling with the steps might be nice, tho im not too sure about it.

Happy to have feedback / a discussion around those points 😊

@codetakki
Copy link
Contributor Author

codetakki commented Oct 29, 2024

Thank you for great feedback!
let me disscuse your points:

  • UI
    • i will adjust it so that the x is always displayed on the Right side of the screen, independent of column
    • I will hide the cook mode symbol above the steps list when in cook mode, so its only the X That is used.
  • The reason i use fullscreen dialog is for more space on mobile, and a more "focused" feel when in cook mode. But i dont see any reason for not displaying it under the app bar instead. Show it if you swipe down, to make it accessible but not take up space (on mobile it takes ca 1/5 of screen space)
  • While i understand that the advantage of doing the extra work is reduced, i would consider this a good thing. If we can improve the usability without the need for extra user work, that's just a win.
    • There is still a advantage as we can more quickly see what steps are used.
    • We could also sort the ingredients after its step on the side, using the number as a group header, and dont display it under the step directly(This could be useful even outside of cook mode)

@Kuchenpirat
Copy link
Collaborator

First of all, thanks for your fast turnaround on my feedback 😊

I would suggest having the ingredients on the left column. This is the layout of the normal recipe view and if there is no strong reason to mix it up in cook mode i would prefer to have have this consistent between cook mode and the normal view.

I think the compromise of the AppBar only showing on swipe up might be a good compromise on small screens. Might be nice to limit this to mobile screens.

#4474 has an screenshot on how tandoor handles ingredients that are linked to steps which i kinda what i was thinking about. This might possibly be out of scope for this PR but i think something like this when linked ingredients are availlable is a nice addition.

@codetakki
Copy link
Contributor Author

This might not be to much to add, I'm thinking the title of the step is used as a header and ingredients appear under it. Plus a header for "other" items

@codetakki
Copy link
Contributor Author

codetakki commented Oct 31, 2024

I have adjusted the cook mode to be contained in the page the same way the normal view is, making the toolbar always visible. Sadly, i was unable to find a way to hide the toolbar in a conditional way that didn't feel too janky, so I just left it, even on smaller screens. A simple way could be to have a toggle for it in cook mode, but i dont like adding extra buttons.

The ingredients section headers feel like i want to make a separate PR. We need to handle cases where ingredients are linked to multiple steps etc

@codetakki
Copy link
Contributor Author

i have added the feature with ingredients being linked to steps:
image

Copy link
Contributor Author

@codetakki codetakki left a comment

Choose a reason for hiding this comment

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

Seams that first title then summary is a pretty good way to do it

@Kuchenpirat
Copy link
Collaborator

Hey, sorry it took me so long to come back to this.

There are still a few things that need improving.

  • On phone view in protrait mode i have it only showing the ingredients and the instructions part bellow is missing.
  • stuff related to linked ingredients
    • ingredients will be shown twice (once bellow the step, and once in the ingredient section)
    • i feel like if we know what step we need the ingredient in it is not necessairy to have the two columns seperately scrollable since we know there each ingredient is supposed to be.
    • We might create a situation with duplicate headings (step 1 + ingredient section. see screenshot bellow)

image

@codetakki
Copy link
Contributor Author

Thank you for valuable feedback.

When we have linked ingredients you want to remove the ingredients column? The we have a Strage situation where we loose functionality when we link them as we can no longer check the off. What's your suggestion to resolving the duplicate issue?

@Kuchenpirat
Copy link
Collaborator

Kuchenpirat commented Nov 6, 2024

Ah there is the missunderstanding,

To be on the same page on what my ideas for an optimal cookmode would be i have designed a rough mock up in figma. This does only depict the situation when the user has linked ingredients as i feel your idea on when this is not the case is what i had in mind also 👍

image

Regarding the duplicate ingredients listing, I'd say we can remove them from directly under the step and have them only shown on the ingredient side. To have them more associated with the step i would remove the individually scrolling part (like in the mock up)

One point against this is, that it will hide ingredient sections, but my feeling is that they are usually in line with the steps (which makes sense), so that would often be duplicate info anyways.

This could also allow us to check off steps and ingredients at the same time (see step 3) & the same could be done for sections. Just to be clear this doesn't have to be part of this PR.

Instead of the lines under the steps we could also have them in individual cards like the instruction steps are right now which might make this more consistent with the rest of the app. Lines were just easier to mock up 😊🙈

@codetakki
Copy link
Contributor Author

Looks good!
Just so I'm understanding this correctly:
If have linked ingredients, we show them like you're mocked. If we don't have it we display steps and ingredients separately, like my current suggestion?

@Kuchenpirat
Copy link
Collaborator

Kuchenpirat commented Nov 6, 2024

yeah 👍
We might have to think about what to do in edgecases (ingredient not linked at all, ingredient linked to multiple steps) but i think this uses the availlable info the best to improve the cookmode experience.

@codetakki
Copy link
Contributor Author

Nice i will look at this once i have time.
What should we do with the ingredients sections? I feel like they might not be needed if they have linked ingredients? The current cook mode ignores the ingredients sections when displaying them below the step

@Kuchenpirat
Copy link
Collaborator

i am not really sure what you mean, but we don't need to have an ingredient section and my mockup which includes the ingredients, that would just again have ingredients on display twice.

@codetakki
Copy link
Contributor Author

Sorry for the unclarity. i was thinking about the sections you can add to ingredients:
image
image
I made test to see how your suggestion would be and i think it looks strange to hav those as it can be duplecates:
image

@Kuchenpirat
Copy link
Collaborator

Kuchenpirat commented Nov 6, 2024

One point against this is, that it will hide ingredient sections, but my feeling is that they are usually in line with the steps (which makes sense), so that would often be duplicate info anyways.

Yeah i suggested leaving them out

@codetakki
Copy link
Contributor Author

I have made changes that i hope match your expectations.
There are effectively two different cook modes now:
Linked ingredients:
image
No linked:
image

In portrait, the ingredients will be placed over the instructions with checkboxes. it would be simple to move them below if that preferable.

image

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.

Hey, first of all thanks for your quick turn around with this and sorry for me taking a bit longer to get back to this. I think we are almost there. Just a few more things.

When i have a recipe with linked ingredients where not all ingredients are linked, the ones that are not linked will not be shown in cookmode.
Recipe Screenshot
image
Screenshot of Recipe in Cookmode missing one ingredient.
image

And can we remove the left margin on the ingredient checkboxes in cookbook view.
With the missing title it looks a bit weird to have them indented.

@codetakki
Copy link
Contributor Author

The current cookMode also does not show ingredients if they are not linked, so I didn't know how we wanted to solve that. My suggestion would be to add a row at the top or bottom of the instructions that says "other" or similar

@Kuchenpirat
Copy link
Collaborator

Kuchenpirat commented Nov 9, 2024

Oh, I never noticed that, but to be honest I never used it much because I didn't like the ui (not being able to check of ingredients mostly), which your PR solves 😊

Your solution sounds good to me. I think at the bottom should be fine (I don't think most people would only link some ingredients).

@codetakki
Copy link
Contributor Author

I have adjusted the margins and added a card with the not linked items

@Kuchenpirat Kuchenpirat self-requested a review November 10, 2024 15:49
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.

This should be the last round of feedback, almost there 😊

frontend/lang/messages/en-US.json Outdated Show resolved Hide resolved
frontend/lang/messages/en-US.json Outdated Show resolved Hide resolved
@codetakki
Copy link
Contributor Author

There we go :D

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.

Let's get this in then 🚀
Really happy with the end result! 😊

Thank you for contributing to mealie and sticking with all the feedback rounds 👍

@Kuchenpirat Kuchenpirat merged commit d419acd into mealie-recipes:mealie-next Nov 11, 2024
13 checks passed
@codetakki
Copy link
Contributor Author

codetakki commented Nov 11, 2024

No problem! Thank you for holding a good standard.
I'm also very happy with the end result and look forward to using it :)
Do you want me to write some kind of changelog or is that managed separately?

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.

3 participants