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: Show recipe tags on mobile view and meal plan #3864

Merged
merged 2 commits into from
Jul 10, 2024

Conversation

AverageMarcus
Copy link
Contributor

What type of PR is this?

  • feature

What this PR does / why we need it:

This change makes it so that tags on a recipe will show in mobile view as well as on the meal plan for both mobile and desktop.

Meal plan - mobile
Screenshot 2024-07-07 at 9 18 05 AM

Meal plan - desktop
Screenshot 2024-07-07 at 9 18 26 AM

Which issue(s) this PR fixes:

N/A

Testing

Manual testing with example recipes. Created recipes with no tags, single tags and multiple (3) tags to ensure all show as expected.

Signed-off-by: Marcus Noble <github@marcusnoble.co.uk>
@boc-the-git
Copy link
Collaborator

My initial thoughts here are how many tags should be displayed (you've capped at 2)? What if people don't want any tags displayed? Are there places other than the meal plan they should be displayed (from a skim it looks like you've only added it there)?
Is there any existing feature request around this with prior discussion?

I don't have good answers here, just dumping some thoughts.

@AverageMarcus
Copy link
Contributor Author

There is currently a place where the tags are shown like this - on the recipe search page when viewed on desktop (ut not mobile, this PR adds that).

I capped at two tags because that was what was currently being used on the desktop view and thought it made sense to be consistent. See here:

<RecipeChips :truncate="true" :items="tags" :title="false" :limit="2" :small="true" url-prefix="tags" />

There isn't a previous feature request. Should I still open one?

@AverageMarcus
Copy link
Contributor Author

It would be nice if this stuff could be configured in setting - e.g. show/hide tags, configure number of tags shown. But that's a larger change that effects other areas too and not something I'm currently comfortable with tackling.

@boc-the-git
Copy link
Collaborator

on the recipe search page when viewed on desktop (ut not mobile, this PR adds that).

Do you know why that isn't displaying on mobile? From a quick look I couldn't see why.. Assuming it's unintentional (not specifically coded for), that could be a good change to add in here?

There isn't a previous feature request. Should I still open one?

No stress, was just wondering if there was previous discussion.

It would be nice if this stuff could be configured in setting - e.g. show/hide tags, configure number of tags shown. But that's a larger change that effects other areas too and not something I'm currently comfortable with tackling.

Any chance you want to raise a feature request for that?

@AverageMarcus
Copy link
Contributor Author

Do you know why that isn't displaying on mobile? From a quick look I couldn't see why.. Assuming it's unintentional (not specifically coded for), that could be a good change to add in here?

Yeah, because on mobile it uses the RecipeCardMobile.vue component which is what I've updated here so they will both be inline. This also happens to be what is used on the mealplan page for both desktop and mobile from what I can see.

Any chance you want to raise a feature request for that?

Good idea. I'll put something together later today hopefully. :)

@boc-the-git
Copy link
Collaborator

@michael-genson @Kuchenpirat , any concerns with this change?

@michael-genson
Copy link
Collaborator

Yeah this looks fine to me, looks like we just never added tags to the mobile card view

@boc-the-git boc-the-git enabled auto-merge (squash) July 10, 2024 09:33
@boc-the-git
Copy link
Collaborator

Thanks Marcus!

@boc-the-git boc-the-git merged commit 6e680c9 into mealie-recipes:mealie-next Jul 10, 2024
11 checks passed
@AverageMarcus AverageMarcus deleted the mobile-tags branch July 10, 2024 09:44
@AverageMarcus
Copy link
Contributor Author

Thank you so much for making my first contribution so pleasent! 😀 Will for sure be looking out for other things I can help improve.

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.

3 participants