-
-
Notifications
You must be signed in to change notification settings - Fork 784
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
Feature: Shopping List Label Section Improvements #2090
Merged
hay-kot
merged 22 commits into
mealie-recipes:mealie-next
from
michael-genson:feat/shopping-list-rendering-improvements
Feb 22, 2023
Merged
Feature: Shopping List Label Section Improvements #2090
hay-kot
merged 22 commits into
mealie-recipes:mealie-next
from
michael-genson:feat/shopping-list-rendering-improvements
Feb 22, 2023
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…list-rendering-improvements
removed unique contraint removed label settings from main route/schema added new route for label settings
moved shopping list label logic to service modified label seeder to use service
I'm not sure why the backup restore failed; it failed once on my end, then I changed nothing, ran all tests again, and it worked. Might be some cache thing? |
…list-rendering-improvements
Tweaked the failing test and it passes now? ¯\_(ツ)_/¯ Still couldn't get it to fail locally |
michael-genson
changed the title
Shopping List Label Section Improvements
Feature: Shopping List Label Section Improvements
Feb 16, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What type of PR is this?
(REQUIRED)
What this PR does / why we need it:
(REQUIRED)
This adds some additional functionality to the label sections on the shopping list, bringing it more inline with other comparable list apps:
Similar to reordering items on a shopping list, reordering labels requires additional backend data to store the position of each label. Since different users may want to order their labels differently, I made this relationship many to many between shopping lists and labels (i.e. each shopping list has its own label order). This new many to many relationship is stored in the
ShoppingListMultiPurposeLabelOut
object, which is referenced on the Shopping List aslabel_settings
. A backend route was added to modify the label settings directly, you cannot modify it in the same call as modifying the shopping list (more info below in the reviewer notes).Here's what the UI looks like for reordering labels. Notice that all labels are present, even if that label doesn't currently appear on the list.
"No label" items will always appear at the top (and the tag icon is removed)
Within each label section you can reorder items. Notice that you can't drag between label sections, you have to edit the item to do that (I'd love to enable that feature, but this is already pushing my JS)
If you have a lot of items (or you're on mobile) the list of labels is scrollable. The whitespace between the text and the drag handle allows mobile users to scroll, but you can drag touching the handle or the label name/icon. Originally I had it so you could touch the entire row, but then mobile users were SOL.
The "Reorder Labels" button that opens the dialog is only present when in the group by labels view. If you're in the default view, the button isn't there (since it would be useless).
Which issue(s) this PR fixes:
(REQUIRED)
N/A
Special notes for your reviewer:
(fill-in or delete this section)
The way the many-to-many relationship is handled is pretty manual, since it contains extra data. I followed the guide from the docs, for the most part, but I had to make some changes due to some of our low-level architecture. Namely, we don't really support composite keys.
Our Sqlalchemy base automatically adds a primary key called "id", and many of our lower-level database operations depend on that. Without completely rewriting some of the lower level stuff (which would be needlessly silly for a feature like this) I stayed away from a composite pk and limited access to the new
ShoppingListMultiPurposeLabelOut
object such that you can only interact with it through an explicit update (as opposed to updating it with the normal shopping list route). It's probably more efficient on the db this way anyway. Originally I tried going the composite key route, but there were too many gotchas that made it not worth it.One thing this change does is gets rid of the colored labels next to the items (which are still present if you don't group by label). We should find a way to incorporate the color back into this view without being redundant. I tried a few things but couldn't land on anything that wasn't gross or tacky. I may play around with it some more in another PR.
Testing
(fill-in or delete this section)
Pytest :)
Release Notes
(REQUIRED)