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

Added kilojoules property to ingredient model (Issue #568) #577

Merged
merged 8 commits into from
Dec 15, 2020

Conversation

nopinter
Copy link
Contributor

Proposed Changes

  • added "energy_kilojoule" property to Ingredient class in wger/nutrition/models.py

  • added kilojoules to display on ingredient details page in wger/nutrition/templates/ingredient/view.html

  • added to wger/nutrition/static/js/nutrition.js, making kilojoules responsive to updates in ingredient amount according to user input

  • added test to wger/nutrition/tests/test_ingredient.py, verifying correct kilojoule conversion

Please check that the PR fulfills these requirements

  • [✓] Tests for the changes have been added (for bug fixes / features)
  • [✓] New python code has been linted with with flake8 (flake8 --config .github/linters/.flake8)
    and isort (isort)
  • [✓] Added yourself to AUTHORS.rst

Other questions

  • Does this PR introduce a breaking change such as a database migration? (i.e.
    what changes might users need to make in their running application due to
    this PR?)

No

@nopinter
Copy link
Contributor Author

I worked on these changes with my partner, @derekli17, which is why his name is also included in AUTHORS.rst. We are currently undergraduates studying computer science, so if you have any feedback or want us to make changes, please feel free to let us know! Thanks.

@rolandgeider rolandgeider linked an issue Dec 13, 2020 that may be closed by this pull request
wger/nutrition/models.py Outdated Show resolved Hide resolved
wger/nutrition/models.py Outdated Show resolved Hide resolved
@rolandgeider
Copy link
Member

rolandgeider commented Dec 13, 2020

While showing everywhere both the kcal and the kJ would break the UI flow, I'd add them to the nutrition list and the nutritional data table under the plan, then this would be ready to merge

@nopinter
Copy link
Contributor Author

Hi, thanks for your feedback! We've implemented your suggested changes. We found the "4,184" / "4.184" issue very funny, but that now is fixed 👍 . Additionally, we wanted your feedback on an aesthetic issue, which we've attached a picture of.

Screen Shot 2020-12-13 at 10 30 05 PM

As you can see in the red circle, sometimes the kilojoules wrap around onto the next line. Let us know your thoughts on this. We'd be more than willing to change some of the HTML/CSS to make it look cleaner if necessary.

@rolandgeider
Copy link
Member

rolandgeider commented Dec 14, 2020

I also had to laugh when I opened a plan and saw like half a million kJ 😂

And as for the line wrapping issue, we can save some space by removing the decimal place but other than that I wouldn't change much

Edit: it has technically nothing to do with this issue, but can you perhaps take a look at #581?

@nopinter
Copy link
Contributor Author

We have experimented with removing the decimal place, but we are still observing the line wrapping issue. We didn't make any further changes for this reason

@nopinter
Copy link
Contributor Author

Also, we would be willing to take #581 instead of #575 ! I'll will comment on those to reflect this

@rolandgeider rolandgeider merged commit ff4fa8d into wger-project:master Dec 15, 2020
@rolandgeider
Copy link
Member

Merged, thanks!!

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.

Add kilojoule values to ingredient
3 participants