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

feat: Shopping list UI overhaul #4077

Closed

Conversation

Wetzel402
Copy link
Contributor

@Wetzel402 Wetzel402 commented Aug 22, 2024

What type of PR is this?

  • feature

What this PR does / why we need it:

The shopping list UI seemed a bit complex. This PR's goal is to...

  1. Simplify the UI by moving buttons to an overflow menu in the upper corner
  2. List items are in v-card now to help with readability
  3. Each label now has it's own add button that should add to that label by default to speed up/simplify adding under that label. (in progress)
  4. Default to the label sorted view rather than the unsorted view
  5. Add keep screen awake
Screenshots

image
image
image

Which issue(s) this PR fixes:

None

Testing

Tested in dev container

    modified:   frontend/pages/group/data/labels.vue
	modified:   frontend/pages/shopping-lists/_id.vue
	modified:   mealie/schema/labels/multi_purpose_label.py
	modified:   docs/docs/documentation/community-guide/home-assistant.md
@Kuchenpirat
Copy link
Collaborator

Gonna leave some early feedback here.
In my opinion putting the items in cards adds a lot of unnecessairy noise to the ui.

I think its also more pronounced when not in dark mode, so additional screenshots bellow.

With Cards Without Cards
image image

The look without them just looks way cleaner.

@Wetzel402
Copy link
Contributor Author

The look without them just looks way cleaner.

I agree especially with the divider per label

@boc-the-git
Copy link
Collaborator

boc-the-git commented Sep 15, 2024

FYI @Wetzel402, this is failing CI, which is the reason I haven't spent any time to review it (and I imagine similar might be true for others). Oh, and it's marked as a draft.

@Wetzel402
Copy link
Contributor Author

FYI @Wetzel402, this is failing CI, which is the reason I haven't spent any time to review it (and I imagine similar might be true for others). Oh, and it's marked as a draft.

I removed the unused variable. The PR is in draft due to the major UI changes. @Kuchenpirat was reviewing the changes and we may still make changes to the PR before it is incorporated fully into mealie:next. There were also a few things I didn't fully finish working on due to my limited knowledge of vue and not knowing if the features would be fully approved or not. I think we should wait for @Kuchenpirat's feedback and go from there.

@Wetzel402 Wetzel402 closed this Sep 16, 2024
@Wetzel402 Wetzel402 reopened this Sep 16, 2024
@Kuchenpirat
Copy link
Collaborator

Kuchenpirat commented Sep 16, 2024

Ah sorry was not aware this was blocked by me. Will review tomorrow during the day👍

@Wetzel402
Copy link
Contributor Author

Ah sorry was not aware this was blocked by me. Will review tomorrow during the day👍

I wouldn't say blocked. I just know you were looking at the UX changes based on our discord discussion. Thanks!

Copy link
Collaborator

@Kuchenpirat Kuchenpirat left a comment

Choose a reason for hiding this comment

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

Looking at all my comments i really suggest just sliming down this PR, focusing on the key elements while addressing the rest in separate PRs. This way, you'll have something merged quickly and can build on it, allowing us to review and discuss specific items more effectively instead of tackling such a broad PR.

I really like the general idea of cleaning up the shopping list interface, but some aspects still need work.

What is ready to go:

  • The visual change of having labels over each category
  • Wake lock

I'm happy to merge a PR with these updates, and we can address the rest in future PRs. 👍

@@ -140,7 +140,7 @@ export default defineComponent({
setup(props, context) {
const { i18n } = useContext();
const displayRecipeRefs = ref(false);
const itemLabelCols = ref<string>(props.value.checked ? "auto" : props.showLabel ? "4" : "6");
const itemLabelCols = ref<string>(props.value.checked ? "auto" : props.showLabel ? "8" : "9");
Copy link
Collaborator

Choose a reason for hiding this comment

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

This makes a lot more sense for long list items 👍

Copy link
Contributor Author

Choose a reason for hiding this comment

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

PR for this change...
#4237

@@ -231,7 +231,7 @@ export default defineComponent({
editLabel.value = item;

if (!editLabel.value.color) {
editLabel.value.color = "#E0E0E0";
editLabel.value.color = "#757575";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any particular reason you changed this?

Copy link
Contributor Author

@Wetzel402 Wetzel402 Sep 19, 2024

Choose a reason for hiding this comment

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

If I remember correctly the new label dividers made visibility poor on one of the themes so I made it a bit darker gray to compensate. I would have to play with it to let you know for sure.

Edit: Yes, in the dark theme readability is an issue so I darkened the color...
image
image
image

Comment on lines +25 to +76
<v-col cols="3" class="text-right">
<BaseButtonGroup
:buttons="[
{
icon: $globals.icons.dotsVertical,
text: '',
event: 'edit',
children: [
{
icon: $globals.icons.contentCopy,
text: $tc('shopping-list.copy-as-text'),
event: 'copy-plain',
},
{
icon: $globals.icons.contentCopy,
text: $tc('shopping-list.copy-as-markdown'),
event: 'copy-markdown',
},
{
icon: $globals.icons.checkboxOutline,
text: $tc('shopping-list.check-all-items'),
event: 'check',
},
{
icon: $globals.icons.checkboxBlankOutline,
text: $tc('shopping-list.uncheck-all-items'),
event: 'uncheck',
},
{
icon: $globals.icons.cog,
text: $tc('general.menu'),
event: 'menu',
color: 'info',
},
{
icon: $globals.icons.delete,
text: $tc('shopping-list.delete-checked'),
event: 'delete',
color: 'error',
},
],
},
]"
@edit="edit = true"
@delete="openDeleteChecked"
@uncheck="openUncheckAll"
@check="openCheckAll"
@copy-plain="copyListItems('plain')"
@copy-markdown="copyListItems('markdown')"
@menu="toggleMenuDialog()"
/>
</v-col>
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this needs some more work.
It might be ok to hide a few things behind the three dot menue, but for some things it gets a bit to much.
E.g. the Owner Settings are now: Three Dot ➡️ Menu ➡️ Settings

My suggestion would be to have that in a later PR, this needs to be a bit more fleshed out.
The thing i am sure that makes sense to put in a three dot menu would be the owner settings menu. Im not too sure about the rest

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will try to separate out the changes as you suggest and we can look at the three dot menu separately.

</v-icon>
</span>
{{ key }}
<!-- Create Item -->
Copy link
Collaborator

Choose a reason for hiding this comment

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

i think it is clear that this needs to be polished quite a bit, but i really like the concept and think that this creation flow is the way to go 👍

@@ -101,6 +164,45 @@
</v-card>
</BaseDialog>

<!-- Menu dialog -->
Copy link
Collaborator

Choose a reason for hiding this comment

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

This needs still some work. Having a button in a three dot menue that leads to a modal with further menue options is not the way to do it.

Comment on lines +308 to +313
v-if="wakeIsSupported"
class="d-print-none d-flex px-2"
:class="$vuetify.breakpoint.smAndDown ? 'justify-center' : 'justify-end'"
>
<v-switch v-model="wakeLock" small :label="$t('recipe.screen-awake')" />
</div>
Copy link
Collaborator

Choose a reason for hiding this comment

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

Having the options for wake lock in the shoping list makes absolutely sense 👍😍

@Wetzel402
Copy link
Contributor Author

Looking at all my comments i really suggest just sliming down this PR, focusing on the key elements while addressing the rest in separate PRs. This way, you'll have something merged quickly and can build on it, allowing us to review and discuss specific items more effectively instead of tackling such a broad PR.

I really like the general idea of cleaning up the shopping list interface, but some aspects still need work.

What is ready to go:

  • The visual change of having labels over each category
  • Wake lock

I'm happy to merge a PR with these updates, and we can address the rest in future PRs. 👍

PRs for these 2 changes:
#4235
#4236

@Wetzel402
Copy link
Contributor Author

PR for defaulting to the label sorted view...
#4238

@Kuchenpirat
Copy link
Collaborator

Will close this as this has been divided up on the following PRs:

The remaining features are not production ready yet. We are happy to get them in potential future PRs 👍

@Wetzel402 Wetzel402 deleted the shopping-list-ui branch September 26, 2024 21:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants