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

Heart Rate example #27

Merged
merged 6 commits into from Jun 15, 2022
Merged

Heart Rate example #27

merged 6 commits into from Jun 15, 2022

Conversation

dariusngo
Copy link
Contributor

@dariusngo dariusngo commented Jun 14, 2022

The existing Range Chart is capable of displaying Apple's heart rate chart in the Health app. Included is an example of a semi-complete version of it.
Simulator Screen Shot - iPhone 13 - 2022-06-13 at 20 08 32
image

Apple's:
image

@vibrazy
Copy link
Contributor

vibrazy commented Jun 14, 2022

Great job, was halfway doing this one myself. I have some sample data for a daily bpm, will share.
I would also make the heartrate data a comparable object so you can easily do min max etc.

Copy link

@moritzsternemann moritzsternemann left a comment

Choose a reason for hiding this comment

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

I've also done some work on this type of chart so I thought I'd point out some small improvement suggestions - basically how my implementation differs from yours.

Plus, I've created some data on a single-day-scale so I'll probably add a variation when this is merged 💪

struct HeartRateRangeChartDetail: View {
@State var barWidth = 10.0
@State var chartColor: Color = .red
@State var isShowingPoints: Bool = false

Choose a reason for hiding this comment

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

This is not used, is it?

Suggested change
@State var isShowingPoints: Bool = false

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in fd0ad50. Thanks!

AxisMarks(values: .stride(by: ChartStrideBy.day.time)) { _ in
AxisTick()
AxisGridLine()
AxisValueLabel(format: .dateTime.weekday(.abbreviated), centered: true)

Choose a reason for hiding this comment

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

The value in Apple's version is aligned to the tick/grid line

Suggested change
AxisValueLabel(format: .dateTime.weekday(.abbreviated), centered: true)
AxisValueLabel(format: .dateTime.weekday(.abbreviated))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in fd0ad50. Thanks!

Comment on lines 55 to 56
@ViewBuilder
private func makeHeader() -> some View {

Choose a reason for hiding this comment

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

I suggest to make this a computed property, just like the customisation view. Also, I don't think this needs to be a ViewBuilder, does it?

Suggested change
@ViewBuilder
private func makeHeader() -> some View {
var header: some View {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in fd0ad50. Thanks!

Comment on lines 65 to 66

}.fontWeight(.semibold)

Choose a reason for hiding this comment

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

Suggested change
}.fontWeight(.semibold)
}
.fontWeight(.semibold)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in fd0ad50. Thanks!

AxisValueLabel(format: .dateTime.weekday(.abbreviated), centered: true)
}
}
.frame(height: 300)

Choose a reason for hiding this comment

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

Suggested change
.frame(height: 300)
.frame(height: 300)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in fd0ad50. Thanks!

x: .value("Day", $0.weekday, unit: .day),
yStart: .value("BPM Min", $0.dailyMin),
yEnd: .value("BPM Max", $0.dailyMax),
width: .fixed(10)

Choose a reason for hiding this comment

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

Should we use 6 here? I think it is what comes closest to Apple's version.

Suggested change
width: .fixed(10)
width: .fixed(6)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in fd0ad50. Thanks!

@Wouter125
Copy link

Wouter125 commented Jun 14, 2022

I was also looking into this one. Not sure if the heart rate should be represented by a BarMark between a high and low. My approach was to use a scatter graph with single dots. The interval in which your heart rate get's measured is so often that all single dots grouped on an hour or so get turned into a bar.

So if it skips out or couldn't measure one of your heart rates in between it will show up as a gap. Most of the time it will be a solid line but I think the current data set doesn't reflect a heart rate.

So a very buggy heart rate measurement would turn into a lot of single dots. You can see this on the mobile watch on the 4th measurement.

@dariusngo
Copy link
Contributor Author

I was also looking into this one. Not sure if the heart rate should be represented by a BarMark between a high and low. My approach was to use a scatter graph with single dots. The interval in which your heart rate get's measured is so often that all single dots grouped on an hour or so get turned into a bar.

So if it skips out or couldn't measure one of your heart rates in between it will show up as a gap. Most of the time it will be a solid line but I think the current data set doesn't reflect a heart rate.

So a very buggy heart rate measurement would turn into a lot of single dots. You can see this on the mobile watch on the 4th measurement.

Yeah the data I used was just random data that isn't even related to actual bpm, which probably wasn't the best idea. I just wanted to see if I could replicate what it looked like from a glance. I think you are right about using a scatter graph, however I haven't tried it out myself.

@atrinh0 atrinh0 merged commit 458a11f into jordibruin:main Jun 15, 2022
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.

None yet

6 participants