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

Fix for several Shopping List bugs #1912

Conversation

michael-genson
Copy link
Collaborator

@michael-genson michael-genson commented Dec 21, 2022

What type of PR is this?

(REQUIRED)

  • bug

What this PR does / why we need it:

(REQUIRED)

This fixes several issues with the shopping list feature. Once #1847 is merged I think I'll take another look at the backend shopping list service because I think there are some additional improvements to be had.

The main issues this PR addresses are around the auto-refresh we implemented fairly recently - it broke... a lot more than I thought it did, as you'll see below. This PR more-or-less restores the shopping list back to how it used to behave for the user, but keeps the auto-refresh intact which required some changes. It also fixes a backend bug where checked items were being merged with unchecked items.

Below are two of the bigger frontend changes:

Editing Items

The current state of the shopping list makes it virtually impossible to modify individual items on multiple fronts, since a refresh overwrites any changes a user is in the middle of making. Here is an example from the current build:
2022-12-21_14h22_19

You can see my changes are just being replaced by the original value. My solution in this PR is to basically copy the item in the component which is edited, and then once the user saves the item the copied item emits up to the main component to replace it.
Not pictured: I'm monitoring the network to make sure we are, in fact, still polling.
2022-12-21_14h25_51

Performing a ton of actions at once

We implemented a loading value to make sure a refresh doesn't occur when we're still doing some work (e.g. updating a recipe reference, which takes a while). However, if you do a ton of things at once, whatever finishes first sets the loading value to False. That leads to behavior like this where items pop in and out (note: in the gif the issue takes 4-5 seconds to appear):
2022-12-21_14h29_30

So I replaced the loading bool with a loadingCounter int. Every list action that pauses loading increments the counter, then decrements it. Refreshes only occur when the loading counter is zero (thus everything is done loading).
Not pictured: network requests for the updated list stop until all the operations are done (the gif continues until all network requests finished and the first refresh occurs)
2022-12-21_14h31_43

Which issue(s) this PR fixes:

(REQUIRED)

N/A

Testing

(fill-in or delete this section)

I testing this much more robustly: adding a ton of recipes at once, incrementing them, decrementing them, editing one item, editing several items, etc. Everything seems nice and responsive.

One drawback of this approach is that when you edit an item, it waits for the backend call to show the updated item to the user (you can see this in the second gif where the update is successful, but it takes a sec). I think this is okay, since it gives the user visual feedback that the change did indeed occur and nothing went wrong, but we lose that instant responsiveness. We could probably change this so the update is shown to the user before the backend call, but I'm unsure if we want to. Maybe I'm over thinking it.

Release Notes

(REQUIRED)

increased shopping list responsiveness
fixed an issue where adding a recipe to a shopping list that has checked items will sometimes uncheck them

@michael-genson
Copy link
Collaborator Author

@hay-kot FYI this should be ready :)

@hay-kot hay-kot merged commit 856a009 into mealie-recipes:mealie-next Jan 8, 2023
@michael-genson michael-genson deleted the fix/various-shopping-list-bugs branch January 8, 2023 18:51
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.

2 participants