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: Shopping list labels reordering dialog #3540

Conversation

p0lycarpio
Copy link
Contributor

What type of PR is this?

(REQUIRED)

  • bug

What this PR does / why we need it:

(REQUIRED)

This PR fixes the modal allowing you to reorganize the labels of a shopping list. As mentioned in #3426, a "Save" button has been added to call the API with the new labels order. The cancel button closes the modal and reactivates the API polling.

Which issue(s) this PR fixes:

(REQUIRED)

Fixes #3426

Special notes for your reviewer:

If we change the order of the labels and cancel, the local state is still modified (this can be seen if we re-edit the order quickly). The next API poll (every 5 seconds) returns the original order. It was chosen not to call refresh() when canceling or saving to limit requests.

Testing

Manually tested on the frontend

Copy link
Collaborator

@michael-genson michael-genson left a comment

Choose a reason for hiding this comment

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

Generally looks good! GH Actions threw two lint warnings which I commented on. Otherwise, the only issue I have is regarding the cancel behavior you mentioned - I would expect cancel to instantly revert my changes in the dialog (what if I make some changes, change my mind, then want to reset?).

What if instead of modifying the list in-place, we make a copy of the list which is displayed in the dialog, overwrite it when we open the dialog (so it resets), then save the "copied" list to the database on save? Check out the "Edit List Item" flow in the ShoppingListItem component, this is more-or-less what we do to achieve the same kind of thing.

frontend/pages/shopping-lists/_id.vue Outdated Show resolved Hide resolved
frontend/pages/shopping-lists/_id.vue Outdated Show resolved Hide resolved
@p0lycarpio
Copy link
Contributor Author

Thanks for your feedback! Indeed, it looks better this way. What do you think now?

Copy link
Collaborator

@michael-genson michael-genson left a comment

Choose a reason for hiding this comment

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

Yeah I like this much better, just one tiny thing and then I think this is good to go. I appreciate the work!

frontend/pages/shopping-lists/_id.vue Outdated Show resolved Hide resolved
semicolon

Co-authored-by: Michael Genson <71845777+michael-genson@users.noreply.github.com>
@michael-genson michael-genson enabled auto-merge (squash) May 4, 2024 20:20
@michael-genson michael-genson merged commit 9fad4a9 into mealie-recipes:mealie-next May 4, 2024
10 checks passed
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.

[BUG] - UX: Re-ordering labels on shopping list
2 participants