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

Add toggleable macro elements summary in category footer to see macro… #884

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

mgrzaslewicz
Copy link

@mgrzaslewicz mgrzaslewicz commented Dec 12, 2024

… elements per meal easily

Before

I found it hard to see how much protein and other macros I should add to particular meal (category). I could achieve that by clicking on calories button and see category nutriments. Then after closing the dialog I could already forget macros values. And it required way too many clicks.

After

There is:

  • new Diary setting called 'Show macros summary per category'. Turned off by default
  • macro nutriments summary next to calories, e.g. 'P15 / F20/ C52 400 kcal'. Prefixes stand for protein, fat, carbohydrate.
  • displaying macro nutriments summary is aligned with goal displaying in diary footer. e.g. if fat is not displayed in the diary footer, it won't be displayed in category summary

image

image

image

I know there are missing translations, but I don't want to invest too much upfront in this pull request without knowing the likelihood of merging it.

package.json Outdated Show resolved Hide resolved
@davidhealey
Copy link
Owner

Do you think there is enough room to put the full name, "Protein", "Carbs", "Fat" rather than individual letters?

@mgrzaslewicz
Copy link
Author

mgrzaslewicz commented Dec 12, 2024

Do you think there is enough room to put the full name, "Protein", "Carbs", "Fat" rather than individual letters?

I don't think so
image

edit: with just 'Carbs' it might fit, but I'm not sure if it looks good
image

edit2: on smaller devices it doesn't fit
image

@davidhealey
Copy link
Owner

Yeah with Carbs it looks ok, could we put the unit after it too?

@mgrzaslewicz
Copy link
Author

mgrzaslewicz commented Dec 12, 2024

Yeah with Carbs it looks ok, could we put the unit after it too?

see my edited comment about smaller devices.
I think to 'fit them all' it could be like 'P 17g / F 3g / C 2g'

@davidhealey
Copy link
Owner

Yeah you're right, any idea if other apps have this feature and how they solve the layout?

@mgrzaslewicz
Copy link
Author

mgrzaslewicz commented Dec 12, 2024

Yeah you're right, any idea if other apps have this feature and how they solve the layout?

RP diet has a meal (category) focus and you can adjust everything easily. That's what I miss in this project.
Plus there is target for each meal which in an easy case would be macro target per day / number of meals, but just seeing macros summary below it might be already good enough
image

@davidhealey
Copy link
Owner

Ok let's work with your original idea, just the letters and no units.

@mgrzaslewicz
Copy link
Author

Ok let's work with your original idea, just the letters and no units.

Ok. Anything else needed apart from translations and package.json to be moved out of PR scope?

@davidhealey
Copy link
Owner

At first glance it all looks good to me.

… elements per meal easily

# Before
I found it hard to see how much protein I should add to particular meal (category).
I could achieve that by clicking on calories button and see category nutriments.
Then after closing the dialog I could already forget macros values.
And it required way too many clicks.

# After
There is:
- in Diary settings, there is a new setting called 'Show macros summary per category'
- macro elements summary next to calories, e.g. 'P15 / F20/ C52 400 kcal'. Prefixes stand for protein, fat, carbohydrate.
@mgrzaslewicz
Copy link
Author

I've improved the feature to align displaying category summary nutriments with goal settings as after using app for some time it just makes sense.

@EmilJunker @davidhealey Can I somehow help you to move forward with this pull request and release?

@davidhealey
Copy link
Owner

Did you resolve all of @EmilJunker's comments? (looks like you did)

@mgrzaslewicz
Copy link
Author

Did you resolve all of @EmilJunker's comments? (looks like you did)

Yes

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