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

[v1.0.0b] - Closing a recipe from edit shows cached recipe initially #2268

Open
5 tasks done
JvdMaat opened this issue Mar 20, 2023 · 6 comments
Open
5 tasks done

[v1.0.0b] - Closing a recipe from edit shows cached recipe initially #2268

JvdMaat opened this issue Mar 20, 2023 · 6 comments
Labels
bug: confirmed bug Something isn't working good first issue Good for newcomers

Comments

@JvdMaat
Copy link

JvdMaat commented Mar 20, 2023

First Check

  • This is not a feature request
  • I added a very descriptive title to this issue.
  • I used the GitHub search to find a similar issue and didn't find it.
  • I searched the Mealie documentation, with the integrated search.
  • I already read the docs and didn't find an answer.

What is the issue you are experiencing?

When editing a recipe, and clicking Close (not saving changes), it shows you the recipe with all the changes you made. Hitting refresh updates it back to the actual recipe without all the changes. Just a visual bug.

Deployment

Docker (Linux)

Deployment Details

No response

@hay-kot hay-kot added bug Something isn't working bug: confirmed labels Mar 31, 2023
@hay-kot hay-kot added the good first issue Good for newcomers label Mar 31, 2023
@eliasvelardezft
Copy link

hi! i'd like to tackle this issue. The expected behaviour is not clear to me, should you prompt the user to save the changes when clicking the close button (like when you attempt to close the tab with unsaved changes)? or should it just close it and display the previous un-changed recipe?

@JvdMaat
Copy link
Author

JvdMaat commented Nov 1, 2023

There is already a prompt to save changes. The issue is that if you select the option to not save changes, the UI will show the changes you made (as if you DID save).
You then have to do a page refresh (F5) to load the actual recipe without the changes.
So it's purely a caching/visual bug. Not sure if it's still present withe the frontend being removed now.

@JvdMaat
Copy link
Author

JvdMaat commented Nov 1, 2023

Actually, I take that back.. There is no prompt about saving.
When you edit a recipe, and then click Close, it just discards the changes. HOWEVER visually the changes are there until you refresh the page.
(I edited a recipe, updated servings from 4 to 2, then clicked close. It did not ask me to confirm changes, but it did then show the recipe listed as 2 servings. Hitting F5 updates it to the true 4 servings it should be)

@eliasvelardezft
Copy link

Yes we agree on that, my question is wether the correct behaviour is to just close the edit page (this would require me to fix the issue of the recipe not going back to its original state) or to prompt the user to save (while also fixing the issue if the user chooses not to save)

@JvdMaat
Copy link
Author

JvdMaat commented Nov 1, 2023

@michael-genson / @hay-kot may have some input from a design perspective on that.
From a user perspective I would think asking if a user is sure they do not want to save changes (if anything was changed) upon clicking Close would be a good thing.
So I would go the prompt user if they are sure they want to close without saving route, and fix the issue if the user does indeed not save. (force a refresh or whatever)

@michael-genson
Copy link
Collaborator

michael-genson commented Nov 2, 2023

So I would go the prompt user if they are sure they want to close without saving route, and fix the issue if the user does indeed not save. (force a refresh or whatever)

Fully agree. FYI for @eliasvelardezft (or anyone tackling the issue) you probably want to copy the existing recipe into a temporary ref, then emit the changes so the recipe actually updates. I had to tackle a similar issue in #1912:

const localListItem = ref(Object.assign({}, props.value));

(Or however you want to fix it really :) )

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug: confirmed bug Something isn't working good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

4 participants