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

Weight all in one insertion #231

Merged
merged 6 commits into from Dec 7, 2021

Conversation

chaptergy
Copy link
Contributor

Resolves #230.

Adds a new button to the weight tracking screen to add all values at once in a single dialog. This also enables adding fat, muscle and water as weight, which is then automatically converted to percentage, since we have now associated an overall weight with that measurement.

image

@brodeurlv
Copy link
Owner

Hi @chaptergy ,

Thank you for your contribution but there are two main issues that make it not yet ready for a commit.

First, in term of UI, adding a (+) is making the UI less intuitive for the single value updates. So to uniformize, I would put a + next to each other value and the new one should have a specific icon showing the difference. I guess an icon with multiple + would be better or something like that Link.

Second, the addition of the percentage value is making the graphs incoherent. The graph should either show KG or Percentage but not both at the same time. So here the solution would be, either to store the % as a kilogram and display only Kg or don't change the way it is store and update the graph to manage percentage properly.
In the rest of the code I have chosen the first solution.

Thank you.
Best regards,

Charles

@brodeurlv brodeurlv self-requested a review November 30, 2021 23:04
@chaptergy
Copy link
Contributor Author

About your second point: I have implemented it so that if one of the values is entered in kg instead of %, it is converted to % before it gets saved, so it eventually all is in percent.

I do see your point about the button being confusing though. I'm think a button for each of the values would clutter it a little too much though, however looking for an icon which better conveys that multiple values are added might help.

@chaptergy
Copy link
Contributor Author

@brodeurlv I have now changed the Icon to this:
image

I hope that is sufficient. Or are there any additional things after my explanation above?

@brodeurlv
Copy link
Owner

Hi @chaptergy , Thanks for the icon. That's really the kind of icon I had in mind.

Concerning my second point, it would be better to store in the database the weight in kg instead of % just for the fact that a weight is an actual measurement while a pourcentage is a reference to another measurement. As an example, if at some point the use wants to remove a weight measurement for any reason, then all the other measures (fat, muscles, water) become meaningless.
Another reason for this would be the coherency with the previous value. As previous values, before your version, where stored in kg, it is simpler to keep it that way. Of course you could address this by migrating all the existing database but although the first reason is enough, this is an additional one ! :)

Also I have a question regarding the behavior of your new dialog box. Do you manage the fact that users might not fill all the fields , for example just the weight and fat but let the rest empty ? I can't look at the code right now.

Thanks.
Best regards,

@chaptergy
Copy link
Contributor Author

Hey @brodeurlv,
it seems I'm still confused about your second point. In your current version, the weight is stored in kg; fat, muscle and water is saved in %. My PR does not change that. It simply adds the option to enter fat, muscle and water in kg, which is then converted to % before being stored. The weight continues to be just kg. Are you suggesting to change it so everything is in kg instead of %? Wouldn't that cause the issues you mentioned of it being incompatible with previous values?

I've thought about the other point you've made at one point, but forgot about it, so very good point. I will update it right away.

@brodeurlv
Copy link
Owner

Humm, I'm sorry ! You are absolutly right, I'm storing in percentages. So... I better understand your approach not to impact to much the current implementation.
From an architecture stand point, my point is still valid, but I won't insist on it as it is adding more impact on the code and that's. So forget about my "second point".

Concerning my last point, thank you for considering this. Also, it means that the weight is mandatory as it is the reference for calculating the %.

Best regards and sorry again for the confusion.

@chaptergy
Copy link
Contributor Author

Allright, updated it to handle empty input fields correctly.

Copy link
Owner

@brodeurlv brodeurlv left a comment

Choose a reason for hiding this comment

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

Thank you @chaptergy .

@brodeurlv brodeurlv merged commit 98fba55 into brodeurlv:master Dec 7, 2021
@brodeurlv brodeurlv added this to To do in 0.20.4 via automation Dec 7, 2021
@brodeurlv brodeurlv moved this from To do to Done in 0.20.4 Dec 7, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

All at once weight tracking
2 participants