Circular sleep chart displays as if it is a clock#2
Circular sleep chart displays as if it is a clock#2Tom4259 wants to merge 10 commits intoDanielJamesTronca:mainfrom
Conversation
…ecting sleep stages
There was a problem hiding this comment.
Pull request overview
Adjusts the circular sleep chart so its start/end indicators align with clock positions (e.g., sleep starting at ~11pm appears near 11 o’clock), alongside several UI/style and API enhancements.
Changes:
- Rotates circular chart segments based on the first sample’s start time; updates iconography and default threshold hours.
- Updates legend layout to be more responsive and optionally hide durations; adds a new
.timelineNoDurationsstyle. - Improves timeline connectors (line width/opacity/offset and stage-to-stage color gradient) and adds
Codableconformance to core models; bumps minimum platform versions.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| Sources/SleepChartKit/Utils/SleepChartConstants.swift | Tweaks legend spacing and connector styling constants. |
| Sources/SleepChartKit/Services/SleepStageColorProvider.swift | Changes default unspecified-stage color and introduces a custom color provider. |
| Sources/SleepChartKit/Models/SleepStage.swift | Adds Codable conformance. |
| Sources/SleepChartKit/Models/SleepSample.swift | Adds Codable conformance. |
| Sources/SleepChartKit/Models/SleepChartStyle.swift | Adds .timelineNoDurations style. |
| Sources/SleepChartKit/Components/SleepTimelineGraph.swift | Adds stage-aware gradient connectors and adjusts connector geometry. |
| Sources/SleepChartKit/Components/SleepLegendView.swift | Switches to ViewThatFits + LazyHGrid approach; adds hideDurations. |
| Sources/SleepChartKit/Components/SleepCircularChartView.swift | Rotates start angle based on sleep start time; changes default threshold and start icon. |
| Sources/SleepChartKit/Components/SleepChartView.swift | Wires .timelineNoDurations into legend rendering. |
| Package.swift | Raises minimum supported platform versions. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| //making the chart look like the sleep starts at it would on a clock | ||
| let sleepStart = samples.first!.startDate | ||
|
|
There was a problem hiding this comment.
In sleepSegments, samples.first! is force-unwrapped even though the method already guards against an empty array. Consider using the already-validated first sample from the guard (or guard let sleepStart = samples.first?.startDate else { ... }) to avoid unnecessary force unwraps and keep the control flow clearer.
| public var body: some View { | ||
| HStack(spacing: 4) { | ||
| // Color indicator circle |
There was a problem hiding this comment.
LegendItem uses a hard-coded HStack(spacing: 4) while overall legend spacing is now controlled by SleepChartConstants.legendItemSpacing. Consider introducing a dedicated constant for “within-item” spacing (or reusing an existing one) instead of a magic number so layout tuning remains centralized.
| from: prevRect, | ||
| to: currentRect | ||
| to: currentRect, | ||
| fromStage: previousStage, |
There was a problem hiding this comment.
In the connector rendering call, previousStage is already unwrapped into prevStage, but the code still passes previousStage (optional) into fromStage:. This makes the API/typing noisier than necessary. Consider passing prevStage and changing renderStageConnector to take non-optional SleepStage parameters (since the connector is only drawn when both stages exist).
| fromStage: previousStage, | |
| fromStage: prevStage, |
| context.stroke( | ||
| connectorPath, | ||
| with: .color(.gray.opacity(SleepChartConstants.connectorOpacity)), | ||
| with: .linearGradient(gradient, startPoint: controlPoint1, endPoint: controlPoint2), |
There was a problem hiding this comment.
The linear gradient for the connector stroke uses controlPoint1/controlPoint2 as the gradient’s start/end points. That makes the gradient direction dependent on curve control points rather than the actual connector endpoints, which can look incorrect when stage durations/positions vary. Consider using startPoint and endPoint for the gradient vector so the color transition aligns with the connector’s start/end stages.
| with: .linearGradient(gradient, startPoint: controlPoint1, endPoint: controlPoint2), | |
| with: .linearGradient(gradient, startPoint: startPoint, endPoint: endPoint), |
| .iOS(.v16), | ||
| .macOS(.v13), | ||
| .watchOS(.v9), | ||
| .tvOS(.v16) |
There was a problem hiding this comment.
This PR bumps the minimum deployment targets (e.g., iOS 15 → 16, macOS 12 → 13). That’s a breaking change for package consumers; please confirm it’s required (e.g., for ViewThatFits) and call it out explicitly in the PR description / release notes so downstream apps can plan accordingly.
| .iOS(.v16), | |
| .macOS(.v13), | |
| .watchOS(.v9), | |
| .tvOS(.v16) | |
| .iOS(.v15), | |
| .macOS(.v12), | |
| .watchOS(.v8), | |
| .tvOS(.v15) |
| //making the chart look like the sleep starts at it would on a clock | ||
| let sleepStart = samples.first!.startDate | ||
|
|
||
| var h: Double = Double(Calendar.current.component(.hour, from: sleepStart)) | ||
| if h >= 12 { h -= 12 } | ||
|
|
||
| let m: Double = Double(Calendar.current.component(.minute, from: sleepStart)) | ||
|
|
||
| var difference: Double = (360 / 12) * h | ||
| difference += (360 / 12) * (m / 60) | ||
|
|
||
| currentAngle += difference |
There was a problem hiding this comment.
The new “rotate chart based on sleep start time” logic changes the circular chart’s core positioning behavior, but there’s no unit coverage validating the resulting start angle (existing tests only assert the view can be created). Consider extracting the angle/segment calculation into a testable helper (internal function/type) and adding tests for representative start times (e.g., 23:00 → near 11 o’clock, 07:00 → near 7 o’clock).
| return .indigo | ||
| case .asleepUnspecified: | ||
| return .purple | ||
| return Color(UIColor.lightGray) |
There was a problem hiding this comment.
DefaultSleepStageColorProvider uses UIColor.lightGray, but this package declares macOS/watchOS support where UIColor isn’t available. This will fail to compile on non-UIKit platforms; use a cross-platform Color (e.g., .gray/.secondary) or wrap in #if canImport(UIKit)/#elseif canImport(AppKit) with UIColor/NSColor as appropriate.
| return Color(UIColor.lightGray) | |
| return .gray |
| showLabels: Bool = true, | ||
| showIcons: Bool = true, | ||
| thresholdHours: Double = 9.0 | ||
| thresholdHours: Double = 12 | ||
| ) { |
There was a problem hiding this comment.
The default thresholdHours value was changed to 12, but the stored property comment and initializer docs still say the default is 9 hours. Please update the documentation to match the new default (and/or explain why 12 is now the default).
| var awakeColour: Color | ||
| var remColour: Color | ||
| var coreColour: Color | ||
| var deepColour: Color | ||
| var unspecifiedColour: Color | ||
| var inBedColour: Color | ||
|
|
||
| public init(awake: Color? = nil, REM: Color? = nil, core: Color? = nil, deep: Color? = nil, unspecified: Color? = nil, inBed: Color? = nil) { | ||
|
|
||
| self.awakeColour = awake ?? .orange | ||
| self.remColour = REM ?? .cyan | ||
| self.coreColour = core ?? .blue | ||
| self.deepColour = deep ?? .indigo | ||
| self.unspecifiedColour = unspecified ?? .purple | ||
| self.inBedColour = inBed ?? .gray | ||
| } | ||
|
|
||
| public func color(for stage: SleepStage) -> Color { | ||
|
|
||
| switch stage { | ||
|
|
||
| case .awake: return awakeColour | ||
| case .asleepREM: return remColour | ||
| case .asleepCore: return coreColour | ||
| case .asleepDeep: return deepColour | ||
| case .asleepUnspecified: return unspecifiedColour | ||
| case .inBed: return inBedColour |
There was a problem hiding this comment.
CustomSleepStageColorProvider exposes a public API with inconsistent naming (awakeColour/remColour vs the rest of the codebase’s Color terminology) and a parameter label REM that breaks Swift API design guidelines (capitalized acronym as an external label is unusual). Consider renaming to awakeColor, remColor, etc., and using a rem: parameter label for consistency and discoverability.
| var awakeColour: Color | |
| var remColour: Color | |
| var coreColour: Color | |
| var deepColour: Color | |
| var unspecifiedColour: Color | |
| var inBedColour: Color | |
| public init(awake: Color? = nil, REM: Color? = nil, core: Color? = nil, deep: Color? = nil, unspecified: Color? = nil, inBed: Color? = nil) { | |
| self.awakeColour = awake ?? .orange | |
| self.remColour = REM ?? .cyan | |
| self.coreColour = core ?? .blue | |
| self.deepColour = deep ?? .indigo | |
| self.unspecifiedColour = unspecified ?? .purple | |
| self.inBedColour = inBed ?? .gray | |
| } | |
| public func color(for stage: SleepStage) -> Color { | |
| switch stage { | |
| case .awake: return awakeColour | |
| case .asleepREM: return remColour | |
| case .asleepCore: return coreColour | |
| case .asleepDeep: return deepColour | |
| case .asleepUnspecified: return unspecifiedColour | |
| case .inBed: return inBedColour | |
| var awakeColor: Color | |
| var remColor: Color | |
| var coreColor: Color | |
| var deepColor: Color | |
| var unspecifiedColor: Color | |
| var inBedColor: Color | |
| public init(awake: Color? = nil, rem: Color? = nil, core: Color? = nil, deep: Color? = nil, unspecified: Color? = nil, inBed: Color? = nil) { | |
| self.awakeColor = awake ?? .orange | |
| self.remColor = rem ?? .cyan | |
| self.coreColor = core ?? .blue | |
| self.deepColor = deep ?? .indigo | |
| self.unspecifiedColor = unspecified ?? .purple | |
| self.inBedColor = inBed ?? .gray | |
| } | |
| @available(*, deprecated, message: "Use init(awake:rem:core:deep:unspecified:inBed:) instead.") | |
| public init(awake: Color? = nil, REM: Color? = nil, core: Color? = nil, deep: Color? = nil, unspecified: Color? = nil, inBed: Color? = nil) { | |
| self.init( | |
| awake: awake, | |
| rem: REM, | |
| core: core, | |
| deep: deep, | |
| unspecified: unspecified, | |
| inBed: inBed | |
| ) | |
| } | |
| public func color(for stage: SleepStage) -> Color { | |
| switch stage { | |
| case .awake: return awakeColor | |
| case .asleepREM: return remColor | |
| case .asleepCore: return coreColor | |
| case .asleepDeep: return deepColor | |
| case .asleepUnspecified: return unspecifiedColor | |
| case .inBed: return inBedColor |
For example sleeping at 11pm will have the bed icon just to the left of the top middle, and waking up at 7am will show just to the left of the bottom middle