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: Height percentiles report (#719) #741

Merged
merged 4 commits into from Dec 25, 2023

Conversation

Lastpixl
Copy link
Contributor

@Lastpixl Lastpixl commented Dec 19, 2023

Hi,

I really like the new WHO weight percentile report... So, here is my contribution proposal: a similar height percentile report (#719).

The implementation is really similar to that of the weight percentile report, it works in a dev environment. The instructions are great, writing this was pleasant! Kudos to the maintainers.
makemessages has generated fuzzy entries for french, I took the liberty of also fixing the few instances unrelated to this change for french.

Thanks for considering it for merging. Feel free to modify it in any way you see fit.

@coveralls
Copy link

coveralls commented Dec 19, 2023

Coverage Status

coverage: 98.791% (-0.2%) from 99.018%
when pulling 8f091a7 on Lastpixl:who-height-percentiles
into d053050 on babybuddy:master.

Copy link
Member

@cdubz cdubz left a comment

Choose a reason for hiding this comment

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

Thanks for contributing! Happy to hear it was easy to get going.

I have just one small CR for a variable naming.

Also could you modify:

self.height = round(uniform(8.0, 12.0), 2)
to pick a starting fake height value that fits the model better? Right now when generating fake data and looking at these new reports we don't see any of the WHO lines because the test data is so much lower. Maybe set this to self.height = round(uniform(45.0, 60.0), 2)?

reports/graphs/height_change.py Outdated Show resolved Hide resolved
reports/graphs/height_change.py Outdated Show resolved Hide resolved
reports/graphs/height_change.py Outdated Show resolved Hide resolved
reports/graphs/height_change.py Outdated Show resolved Hide resolved
reports/graphs/height_change.py Outdated Show resolved Hide resolved
@cdubz cdubz added the enhancement Feature requests or improvements to existing functionality label Dec 22, 2023
Lastpixl added a commit to Lastpixl/babybuddy that referenced this pull request Dec 23, 2023
Lastpixl added a commit to Lastpixl/babybuddy that referenced this pull request Dec 23, 2023
@Lastpixl
Copy link
Contributor Author

Thanks for the review, that was a lot of copypasta mistakes on my part... I have applied the requested changes, fixed an additional bug in my last commit.

@Lastpixl
Copy link
Contributor Author

Should I rebase this against master and update translation files to fix the conflict with locale/zh/LC_MESSAGES/django.po?

@cdubz
Copy link
Member

cdubz commented Dec 23, 2023

Should I rebase this against master and update translation files to fix the conflict with locale/zh/LC_MESSAGES/django.po?

Oh shoot yeah I forgot this PR also had locale changes. Yes you’ll need to merge in or rebase on master.

Lastpixl added a commit to Lastpixl/babybuddy that referenced this pull request Dec 23, 2023
@Lastpixl
Copy link
Contributor Author

Rebased on master, re-ran gulp makemessages -a then gulp compilemessages as part of the rebase of the first commit, the one that adds new strings.

@Lastpixl
Copy link
Contributor Author

One more rebase to fix formatting

@cdubz
Copy link
Member

cdubz commented Dec 25, 2023

Looks good. Thanks for contributing! 🎅

@cdubz cdubz merged commit bdeb149 into babybuddy:master Dec 25, 2023
17 checks passed
cdubz pushed a commit that referenced this pull request Dec 25, 2023
cdubz pushed a commit that referenced this pull request Dec 25, 2023
@cdubz cdubz added this to the 2.2.0 milestone Dec 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Feature requests or improvements to existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants