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 Height, Head Circumference, and BMI #360

Merged
merged 4 commits into from Dec 31, 2021

Conversation

daegalus
Copy link
Contributor

@daegalus daegalus commented Dec 29, 2021

Closes #191

I am an expecting father, and learned these are expected metrics that get checked at every doctors visit on a newborn. We figure we can track some of these ourselves at home.

I found BabyBuddy its amazing, and will be contributing more (planning to tackle Medicine/Immunizations/Shots next (just tracking, no notifications/alerts). But figured I would get my feed wet with this task.

Changes in this PR:

  • Added Height, Head Circumference, and BMI (following existing patterns from weight)
  • Added Reports for all 3
  • Updated Statistics dashboard card with new entries.
  • Created a new navigation dropdown Measurements and moved Temperature and Weight into that menu, alongside the new measurements.
  • Added links to the reports on the dropdown for reports.
  • Added new icons to Fontello and updated files accordingly
  • Updated Django localization file for en_GB.
  • Updated tests to include changes.
  • Updated OpenAPI Schema file.
  • Generated new migrations for the changes.
  • Ran all tests to make sure they pass.

Let me know if I missed anything.

@cdubz cdubz temporarily deployed to babybuddy-pr-360 December 29, 2021 13:27 Inactive
@cdubz
Copy link
Member

cdubz commented Dec 29, 2021

@daegalus thanks for contributing! I have been hesitant to add anything more than the "weight" and "temperature" measurements stuff but it looks like you've taken a good approach for it here and done an impressive first pass of work.

I have approved the workflow runs and there are a few linting issues to be resolved there.

I have also approved the Heroku branch deployment so we can see this in action at https://babybuddy-pr-360.herokuapp.com. I ran the fake command in that environment and it worked but it didn't add any of the new data. I see that you added the methods to create the data but you need to also add calls to those methods in _add_child_data for the data to be populated with each fake child.

For review, I'm traveling right now so not quite sure when I'll be able to really dig in to this. I'll also ping some of the regular contributors/users on Gitter for review/opinions.

Re: medical information -- I'm on board to add that as well but lets keep notifications in mind, at least. See #66, #153, #200.

See also the discussion in #217.

@cdubz cdubz self-requested a review December 29, 2021 13:42
@cdubz cdubz added the enhancement Feature requests or improvements to existing functionality label Dec 29, 2021
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.

Well a big PR but easy enough to review when its so well done. Thanks!

Just two review comments to look at and the linting issue and this should be good.

openapi-schema.yml Show resolved Hide resolved
babybuddy/management/commands/fake.py Outdated Show resolved Hide resolved
@daegalus
Copy link
Contributor Author

@daegalus thanks for contributing! I have been hesitant to add anything more than the "weight" and "temperature" measurements stuff but it looks like you've taken a good approach for it here and done an impressive first pass of work.

I have approved the workflow runs and there are a few linting issues to be resolved there.

I have also approved the Heroku branch deployment so we can see this in action at https://babybuddy-pr-360.herokuapp.com. I ran the fake command in that environment and it worked but it didn't add any of the new data. I see that you added the methods to create the data but you need to also add calls to those methods in _add_child_data for the data to be populated with each fake child.

For review, I'm traveling right now so not quite sure when I'll be able to really dig in to this. I'll also ping some of the regular contributors/users on Gitter for review/opinions.

Re: medical information -- I'm on board to add that as well but lets keep notifications in mind, at least. See #66, #153, #200.

See also the discussion in #217.

NP, I will take care of these now. And I have some ideas on how we can do Notifications now with the current structure, while you guys work on the new generic model design. That way we can get some of these features added before 2.0's rewrite.

I would help but I am not familiar with Django, and even if I am a backend/devops engineer, its not something I work in regularly so not familiar with what the idiomatic ways of doing object models is for Python/Django. I live very deep in Go land.

@cdubz cdubz temporarily deployed to babybuddy-pr-360 December 30, 2021 03:33 Inactive
@cdubz
Copy link
Member

cdubz commented Dec 30, 2021

Oops — #361 just added a migration. You’ll need to update yours to follow that one.

@daegalus daegalus force-pushed the feature/add_more_measurements branch from 6af8174 to 741c6e0 Compare December 31, 2021 01:14
@cdubz cdubz temporarily deployed to babybuddy-pr-360 December 31, 2021 01:14 Inactive
@daegalus
Copy link
Contributor Author

Oops — #361 just added a migration. You’ll need to update yours to follow that one.

should be fixed

@daegalus
Copy link
Contributor Author

also noticed the font doesn't seem to be updating on the Heroku deploy, as the icons for the new menu options aren't there. Might need a hard refresh?

@coveralls
Copy link

coveralls commented Dec 31, 2021

Coverage Status

Coverage decreased (-0.1%) to 98.085% when pulling ad4c90b on Daegalus:feature/add_more_measurements into 89ed408 on babybuddy:master.

@cdubz cdubz temporarily deployed to babybuddy-pr-360 December 31, 2021 13:53 Inactive
@cdubz cdubz temporarily deployed to babybuddy-pr-360 December 31, 2021 14:13 Inactive
@cdubz
Copy link
Member

cdubz commented Dec 31, 2021

Looks great! Re: the icons -- when adding new ones it still necessary to run gulp updatestatic as well to pull in the CSS for the new icons.

Thanks, @daegalus!

@cdubz cdubz merged commit f174971 into babybuddy:master Dec 31, 2021
@cdubz cdubz added this to the v1.9.4 milestone Dec 31, 2021
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.

Feature Request: Track baby height
3 participants