-
-
Notifications
You must be signed in to change notification settings - Fork 775
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 extra button "Next step" to ingredient linker dialog #2920
Add extra button "Next step" to ingredient linker dialog #2920
Conversation
clicking button will save current step ingredient links and show the next step in the dialog
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.
This is pretty great. Thanks for the Work.
Some verry small feedback. Otherwiese this looks pretty good to me
frontend/components/Domain/Recipe/RecipePage/RecipePageParts/RecipePageInstructions.vue
Outdated
Show resolved
Hide resolved
frontend/components/Domain/Recipe/RecipePage/RecipePageParts/RecipePageInstructions.vue
Outdated
Show resolved
Hide resolved
Comments 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.
Your problem actually happens without this change for me already: auto and save are under each other (As I use a different local which has longer strings for these buttons). I already thought about this happening for languages where "next step" is a lot longer. I do have some ideas to make the UI better, but mostly for desktop: Except for the small padding fix (which can belong in this PR), I'd personally keep the other notes (like moving the "auto" button) to a different PR. |
After looking at it a bit more I agree with your assessment. And then we need a future pr to rework the buttons in this dialog. |
<BaseButton class="mb-1" color="info" @click="autoSetReferences"> | ||
<template #icon> {{ $globals.icons.robot }}</template> | ||
{{ $t("recipe.auto") }} | ||
</BaseButton> | ||
<BaseButton class="ml-2" save @click="setIngredientIds"> </BaseButton> | ||
<BaseButton class="ml-2 mb-1" save @click="setIngredientIds"> </BaseButton> | ||
<BaseButton v-if="availableNextStep" class="ml-2 mb-1" @click="saveAndOpenNextLinkIngredients"> |
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.
I would use class="my-1" instead of mb-1 as this keeps the button on the same plane as the cancel button and gives us a bit more room around the buttons making them less crammed and easier to tap the right button on mobile.
thanks for this. This really improves the ingredient linking 'flow'. |
What type of PR is this?
What this PR does / why we need it:
Currently, linking ingredients is a lot of work, with a lot of clicks. This reduces the clicks a ton.
This is definitely not the only step to take to make this feature nicer (see #2835), but already fixes the biggest issue in a straightforward manner
Special notes for your reviewer:
I could not figure out a clean (and working) way to reset the scrollbar to the top. I made it so the dialog gets unloaded when closed (with v-if) and when clicking "Next Step", the dialog is closed and opened, making the scrollTop reset.
It does look a bit ugly (the UI flashes), but is a lot better in usability. This "bug" also only happens for this new button. No old features are impacted as far as I can tell.
Testing
Go to any recipe and link ingredients. Scroll down and click the "Next Step" button.
This button does not appear when linking on the last recipe step.
The button should save your changes and show the next step to link.