Skip to content

feat(timeline): add x-axis labels to Deep Timeline - #2445

Closed
andigandhi wants to merge 2 commits into
ryanbr:mainfrom
andigandhi:deep-timeline-x-axis
Closed

andigandhi wants to merge 2 commits into
ryanbr:mainfrom
andigandhi:deep-timeline-x-axis

Conversation

@andigandhi

Copy link
Copy Markdown

What this PR does

Currently, the deep timeline does not have x-axis labels. This makes chart analysis very hard.
This pull request adds x-axis lables which scale according to the zoom level of the graph.

Type of change

  • Bug fix
  • New feature
  • Refactor / cleanup
  • Documentation
  • CI / tooling

How it was tested

Android app was tested on a real device. The scaling of the lables seemd good.
I was not able to test the changes in the iOS App!

Checklist

  • Swift package tests pass for any package I touched (swift test in Packages/<name>)
  • Android unit tests pass if I touched android/ (./gradlew testFullDebugUnitTest)
  • No new build warnings introduced
  • UI changes use only StrandDesign tokens — no hardcoded colors, fonts, or spacing
  • No hardcoded hex frame bytes; protocol facts live in the schema / decoders
  • Follows the conventions in docs/CONTRIBUTING.md
  • I did not commit generated output (Strand.xcodeproj/) or any secrets/keystores

@ryanbr

ryanbr commented Sep 27, 2026

Copy link
Copy Markdown
Owner

Landed in #2520 as your two commits, authorship and messages unchanged. Thanks for this, and for saying outright that you could not test the Apple side: that is what sent me looking at the shared chart rather than assuming it.

I stacked one follow-up commit, for something the diff cannot show on its own. chartTimeTicks is shared with the Today HR card, and Today does not pass a nominal window: it passes visBuckets.first().bucket .. visBuckets.last().bucket, the rendered extent of whatever buckets are banked. So the tiers below an hour reached Today too, and a morning holding ten minutes of HR drew ten 1-minute dotted gridlines on a small card. The label strip skips a label that would collide; the gridlines have no such skip, so they all land.

The sub-hour tiers now sit behind a deepZoom flag, off by default and set only by the Deep Timeline, which is the surface that actually zooms. That also makes the comment just above the thresholds true again, the one promising that a window covering slightly less than nominal still lands on its intended interval. Your thresholds themselves are unchanged.

On Apple I keyed that flag on zoomBounds rather than zoomDomain, which is a trap this file has already sprung once: zoomBounds is supplied once by the zooming call sites, while zoomDomain stays nil until the first pinch, so keying on it would have given a freshly-opened Deep Timeline the static tiers and then switched them under the user mid-gesture (#829 was the follow-up to exactly that).

Two smaller things in the same commit. The Swift helper was building a DateFormatter on every call and formatting a label for each tick, and xTicks discarded every one of them with .map { $0.date }; since it is read inside body, that ran once per frame during a pinch. It now returns dates and formats nothing. And its doc no longer says the platforms read identically, because only the positions are: Apple draws labels with AxisValueLabel() in the viewer's locale, while your Kotlin side formats HH:mm itself, so the two genuinely differ in a 12-hour locale.

I added ChartTimeTicksTests on the Swift side to match your Kotlin one. Eight thresholds written twice in two languages is the kind of duplication that drifts silently, and your Android test was carrying that weight alone.

One consequence worth writing down for whoever reads this later: because OverviewHRChart is shared, this also changes the Today card's Apple axis from .automatic(desiredCount: 5) to round wall-clock ticks. I kept that deliberately, since Android's Today card already ticks that way, so it closes a platform difference instead of opening one.

@ryanbr ryanbr closed this Sep 27, 2026
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.

2 participants