Skip to content

Refactor Sleep UI sections#734

Open
tigercraft4 wants to merge 1 commit into
ryanbr:mainfrom
tigercraft4:refactor/sleep-ui-sections
Open

Refactor Sleep UI sections#734
tigercraft4 wants to merge 1 commit into
ryanbr:mainfrom
tigercraft4:refactor/sleep-ui-sections

Conversation

@tigercraft4

Copy link
Copy Markdown

What this PR does

Moves three remaining Sleep screen UI clusters into sibling files:

  • SleepNightNavUi.kt for the sleep window row and night navigation/edit/delete controls.
  • SleepMetricCardsUi.kt for the metric grid, sleep debt ledger, stage comparison, trend cards, and shared Sleep chart helpers.
  • SleepMetricDetailSheet.kt for the metric detail bottom sheet.

This keeps the existing comments with the code they describe and reduces SleepScreen.kt to about 2.1k lines without changing sleep calculations, storage, BLE behavior, or screen wiring. Two delete-dialog body strings were moved to Android resources so the i18n gate stays clean after the file move.

Type of change

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

How it was tested

  • git diff --check
  • python3 Tools/i18n_audit.py --ci origin/main
  • JAVA_HOME=/opt/homebrew/Cellar/openjdk@21/21.0.11/libexec/openjdk.jdk/Contents/Home ANDROID_HOME=$HOME/Library/Android/sdk ./gradlew --dependency-verification=off :app:compileFullDebugKotlin from android/

Android unit tests were not run for this UI-only extraction; the current test task is already blocked by the existing DeviceRegistryDao.setModel(id, model) fake DAO mismatch tracked separately.

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

Related issues

Refs #665

@ryanbr ryanbr left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

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

The extraction itself looks good — a genuine behavior-preserving move, and the SleepChartCard/SleepChartFooter/SleepMetricsHairline rename + internal promotion is the right way to let the moved code reach the former private helpers. Scope is UI-only (no analytics/storage/wiring touched). Two things block it, and the first means it needs a re-review after you update it.

1. Needs a rebase — and it's not a trivial one

The PR is currently CONFLICTING. SleepScreen.kt changed on main after this branched — #748 (compact metric-summary layouts) and #743 (compact range selectors) both edited Sleep UI. Since this PR deletes ~1,162 lines of that same file, please re-apply the extraction on top of the current SleepScreen.kt rather than resolving the conflict mechanically — a blind merge here can silently drop #748/#743's changes. The resulting diff will be meaningfully different, so I'll give it a fresh pass once it's rebased.

2. pt-PT coverage for the two moved strings

The green i18n check is stale — it ran before #769 made pt-PT a focus locale. The two delete-dialog strings you moved into resources are in de/es/fr but not values-pt-rPT, so after rebasing onto current main the gate fails with 2 pt-PT gaps:

  • sleep_delete_user_edited_body
  • sleep_delete_recorded_body

Add both to android/app/src/main/res/values-pt-rPT/strings.xml (Portuguese) as part of the rebase.

Note

Unit tests weren't run (the DeviceRegistryDao.setModel fake-DAO blocker, tracked separately). It's a UI-only move so behavioral risk is low, but the rebase is exactly where a large move-refactor can go wrong — worth a compileFullDebugKotlin + a manual pass over the Sleep screen after re-applying it on current main.

Happy to re-review as soon as it's rebased with the pt-PT strings added.

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