Add HR Zones in activity #199
Conversation
|
Hi, thanks for the PR. This makes totally sense. I will review this ASAP :) What do you think of changing this to a pie chart or a bar chart? |
There was a problem hiding this comment.
Pull Request Overview
This PR adds heart rate (HR) zone calculations and display to activities, based on user birthdate.
- Frontend: passes activity streams to the summary component and adds total/moving times and HR zone UI.
- Backend: introduces a transform step in the activity streams CRUD to calculate
hr_zone_percentagesusing NumPy. - i18n: adds new translation keys for HR and zones.
Reviewed Changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| frontend/app/src/views/ActivityView.vue | Passes new activityActivityStreams prop to the summary component |
| frontend/app/src/components/Activities/ActivitySummaryComponent.vue | Displays moving/total time and HR zones; adds hrZones state and getZoneColor helper |
| frontend/app/src/i18n/us/components/activities/activitySummaryComponent.json | Adds translation strings for HR and zone labels |
| backend/app/activities/activity_streams/schema.py | Adds hr_zone_percentages field to the Pydantic schema |
| backend/app/activities/activity_streams/crud.py | Applies transform_activity_streams_hr to calculate HR zones and updates API returns |
| backend/app/activities/activity_streams/constants.py | Defines stream type constants (including unused STREAM_TYPE_HR_ZONES) |
Comments suppressed due to low confidence (2)
frontend/app/src/views/ActivityView.vue:6
- Prop
activityActivityStreamsis passed but not defined in this component. Ensure you fetch the activity streams inActivityView.vueand expose them asactivityActivityStreamsto avoid undefined prop errors.
:activityActivityStreams="activityActivityStreams"
frontend/app/src/components/Activities/ActivitySummaryComponent.vue:254
- The prop
activityActivityStreamsis declared with type[Object, null]but is used as an array. Update its type toArray(e.g.,type: Array) to reflect correct usage.
activityActivityStreams: {
type: [Object, null],
required: true,
},
frontend/app/src/components/Activities/ActivitySummaryComponent.vue
Outdated
Show resolved
Hide resolved
| hrZones.value = props.activityActivityStreams.find(stream => stream.hr_zone_percentages).hr_zone_percentages || {}; | ||
|
|
There was a problem hiding this comment.
Using onMounted to set hrZones means updates to activityActivityStreams later won’t recalculate zones. Consider using a watch or computed property to react to prop changes.
| hrZones.value = props.activityActivityStreams.find(stream => stream.hr_zone_percentages).hr_zone_percentages || {}; |
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
should be fine, i dont expect the value to change over time, only the first api call
|
|
Awesome. I would just add the percentage somewhere to make it more easily read. |
|
|
|
Nice! Really like it |
- return transform_activity_streams to get_public_activity_stream_by_type -
[backend] fixed formatting [backend] changed existing docstrings to be more complete [backend] changed imports to be consistent with rest of the code [frontend] fixed formatting [frontend] set new prop on ActivitySummaryComponent to be not required because it is not always necessary (HomeView) [frontend] fixed issue onMounted on ActivitySummaryComponent when new prop is null
[frontend] moved HR Zones graph away from ActivitySummaryComponent [frontend] removed duplicated translation [frontend] reverted changes on ActivitySummaryComponent since chart was removed [frontend] moved chart to ActivityMandAbovePillsComponent [frontend] small adjustments to BarChart
|
Hi! Moved chart to the Graphs section to be aligned with current logic. In the future with other graphs of this type (power for example) this can be revisited. What do you think? |
[frontend] moved chart functions to chartUtils file [frontend] added HR Zones Bar Chart to mobile view
|
Merged on pre-release |
Description
this PR adding hr zones calculation based on user birthdate (see https://www.heartonline.org.au/resources/calculators/target-heart-rate-calculator