DAY-190: Make portrait layouts content-size resilient#271
DAY-190: Make portrait layouts content-size resilient#271FleetAdmiralJakob wants to merge 8 commits into
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds centralized content-size calculations and applies viewport- and font-scale-aware padding, stacking, text wrapping, scrolling, and geometry updates across shared UI components, app screens, learning-plan flows, and authentication onboarding. ChangesResponsive content-size layout rollout
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Visual evidenceHome — controlled iPhone 16 Accessibility Medium comparisonSame authenticated simulator, account, Home data, portrait viewport, top scroll position, and fixed 09:41 status bar. Only the JavaScript bundle changes from
Before, the notification control is cut by the right edge, the date and time collide, and the calendar row escapes both sides of its card. After, controls stay in bounds, the date and time separate cleanly, and larger content continues by vertical scrolling instead of being compressed. Android Settings — before
Android Settings — after
iOS iPad AXXXL — full 96.1-second timeline
iOS iPad AXXXL — condensed highlight
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/app/entry/new.tsx (1)
903-908: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAlign sticky footer padding with scrollable content.
Both
PortraitContentwrappers use a hardcodedpx-6(24px padding) rather than the dynamically computedhorizontalPaddingused by theScrollView. This causes the action buttons to be misaligned with the form fields.
src/app/entry/new.tsx#L903-L908: RemoveclassName="px-6"and addpaddingHorizontal: horizontalPaddingto thestyleobject.src/app/entry/new.tsx#L937-L942: Make the same change for the non-homework branch.💡 Proposed fix
- <PortraitContent - className="px-6" - style={{ - paddingBottom: Math.max(insets.bottom + 10, 24), - }} - > + <PortraitContent + style={{ + paddingHorizontal: horizontalPadding, + paddingBottom: Math.max(insets.bottom + 10, 24), + }} + >🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/entry/new.tsx` around lines 903 - 908, Update both PortraitContent wrappers in src/app/entry/new.tsx at lines 903-908 and 937-942: remove the hardcoded className="px-6" and add paddingHorizontal: horizontalPadding to each style object, preserving the existing paddingBottom behavior.src/features/auth/dayova-auth-flow.tsx (1)
1964-1989: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winMake the creation state scrollable for enlarged text.
This fixed
Viewcan overflow on short devices when the system font enlarges the multiline message. Use aScrollViewso the loader remains reachable instead of clipping content.Proposed fix
- <View + <ScrollView + bounces={false} + showsVerticalScrollIndicator={false} style={{ - flex: 1, alignSelf: "center", maxWidth: contentSizeLayout.containerMaxWidth, + width: "100%", + }} + contentContainerStyle={{ + flexGrow: 1, paddingTop: Math.max(topInset + 24, 36), paddingBottom: Math.max(bottomInset + 22, 32), paddingHorizontal: contentSizeLayout.horizontalPadding, alignItems: "center", justifyContent: "center", - width: "100%", }} > <AnimatedFlower /> <Text ...> Dein persönliches Lernprofil{"\n"}wird nun für dich erstellt. </Text> - </View> + </ScrollView>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/features/auth/dayova-auth-flow.tsx` around lines 1964 - 1989, The Lernprofil creation-state container around AnimatedFlower and the multiline Text is not scrollable, so enlarged fonts can clip its content. Replace the fixed inner View with a ScrollView while preserving its safe-area, width, alignment, and spacing styles, ensuring the loader and message remain reachable on short devices.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/app/`(app)/home.tsx:
- Around line 1677-1684: Update the DayNavigationHandle style in the
shouldStackInlineContent branch so its horizontal position includes the outer
centering offset used by the capped 480-point portrait tablet composition,
rather than anchoring at left: 0. Preserve the existing top positioning, scale,
and navigation callbacks.
In `@src/components/ui/select-sheet.tsx`:
- Around line 100-101: Update the BottomSheetModal style near the sheetWidth
usage so the absolutely positioned BottomSheetBody receives symmetric horizontal
insets, centering the fixed tablet width; do not rely on alignSelf alone.
Preserve the existing sheetWidth sizing and vertical behavior.
In `@src/features/auth/dayova-auth-flow.tsx`:
- Around line 2290-2297: Update the carousel sizing logic near valueBadgeSize to
derive carouselWidth from the onboarding container’s usable width after
horizontal padding, rather than directly from the full viewport width. Ensure
the resulting width is capped at 360px and that sidePadding and snapping remain
centered without clipping.
---
Outside diff comments:
In `@src/app/entry/new.tsx`:
- Around line 903-908: Update both PortraitContent wrappers in
src/app/entry/new.tsx at lines 903-908 and 937-942: remove the hardcoded
className="px-6" and add paddingHorizontal: horizontalPadding to each style
object, preserving the existing paddingBottom behavior.
In `@src/features/auth/dayova-auth-flow.tsx`:
- Around line 1964-1989: The Lernprofil creation-state container around
AnimatedFlower and the multiline Text is not scrollable, so enlarged fonts can
clip its content. Replace the fixed inner View with a ScrollView while
preserving its safe-area, width, alignment, and spacing styles, ensuring the
loader and message remain reachable on short devices.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 02694a1f-cd44-4a74-a29e-58d97ccb3109
📒 Files selected for processing (41)
docs/contexts/design-system/CONTEXT.mdsrc/app/(app)/home.tsxsrc/app/(app)/learning-plans.tsxsrc/app/(app)/plans.tsxsrc/app/(app)/settings.tsxsrc/app/entry/[id].tsxsrc/app/entry/new.tsxsrc/app/learning-plans/[planId]/analysis.tsxsrc/app/learning-plans/[planId]/generating.tsxsrc/app/learning-plans/[planId]/index.tsxsrc/app/learning-plans/[planId]/quiz/[questionIndex].tsxsrc/app/learning-plans/[planId]/review.tsxsrc/app/learning-plans/[planId]/sessions/[sessionId]/edit.tsxsrc/app/learning-plans/[planId]/sessions/[sessionId]/index.tsxsrc/app/learning-plans/new.tsxsrc/app/learning-times/edit.tsxsrc/app/learning-times/index.tsxsrc/app/notification-settings.tsxsrc/app/notifications.tsxsrc/app/profile.tsxsrc/app/validation/overview.tsxsrc/components/screen-header.tsxsrc/components/ui/bottom-modal.tsxsrc/components/ui/button.tsxsrc/components/ui/date-time-picker-sheet.tsxsrc/components/ui/list-row.tsxsrc/components/ui/notched-action-card-layout.test.tssrc/components/ui/notched-action-card-layout.tssrc/components/ui/notched-action-card.tsxsrc/components/ui/portrait-content.tsxsrc/components/ui/screen.tsxsrc/components/ui/select-sheet.tsxsrc/components/ui/success-confirmation-screen.tsxsrc/components/ui/warning-banner.tsxsrc/features/auth/dayova-auth-flow.tsxsrc/features/learning-plans/learning-plan-ui.tsxsrc/features/learning-plans/quiz-step.tsxsrc/lib/content-size-layout.test.tssrc/lib/content-size-layout.tssrc/lib/learning-path-layout.test.tssrc/lib/learning-path-layout.ts
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/app/learning-plans/[planId]/review.tsx (1)
280-288: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReplace hardcoded padding with the computed layout padding.
The hardcoded
px-10class (40px) on the bottom action container bypasses the responsivehorizontalPaddinglayout contract used on theScrollViewabove. This will cause the footer actions to misalign with the main content horizontally on smaller devices or when accessibility font scaling adjustments are active.Pass
horizontalPaddingvia thestyleprop to ensure consistent alignment.🐛 Proposed fix
<PortraitContent className={cn( - "gap-3 px-10", + "gap-3", shouldStackInlineContent ? "items-stretch" : "flex-row items-center", )} // Safe-area padding is runtime device data. - style={{ paddingBottom: Math.max(insets.bottom + 24, 36) }} + style={{ + paddingHorizontal: horizontalPadding, + paddingBottom: Math.max(insets.bottom + 24, 36), + }} >🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/learning-plans/`[planId]/review.tsx around lines 280 - 288, Update the PortraitContent container’s styling to remove the hardcoded px-10 horizontal padding and include the computed horizontalPadding in its style alongside the existing safe-area paddingBottom. Preserve the current gap, alignment, and conditional flex classes while reusing the horizontalPadding contract from the surrounding ScrollView.
🧹 Nitpick comments (1)
src/app/learning-plans/[planId]/review.tsx (1)
306-309: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winEnsure a minimum touch target height when stacked.
When
shouldStackInlineContentis true, the primary action button correctly loses theh-14fixed height constraint so it can expand vertically if the text wraps. However, addingmin-h-14guarantees a minimum 56px touch target size for accessibility, preventing the button from potentially shrinking below standard interaction thresholds.♻️ Proposed refactor
className={cn( "min-w-0", - shouldStackInlineContent ? "w-full" : "h-14 flex-1", + shouldStackInlineContent ? "w-full min-h-14" : "h-14 flex-1", )}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/app/learning-plans/`[planId]/review.tsx around lines 306 - 309, Update the className conditional for the primary action button so the stacked shouldStackInlineContent branch includes min-h-14 while remaining vertically expandable, preserving the existing h-14 behavior for the non-stacked branch.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/features/auth/auth-content-size-layout.test.ts`:
- Around line 24-32: Rename the test describing getOtpCellLayout to state that
the maximum-size OTP geometry exceeds the available width, rather than claiming
the row is scrollable. Keep the existing width calculation assertion, and add or
update a component test to render the OTP row and assert its horizontal
scrolling behavior.
---
Outside diff comments:
In `@src/app/learning-plans/`[planId]/review.tsx:
- Around line 280-288: Update the PortraitContent container’s styling to remove
the hardcoded px-10 horizontal padding and include the computed
horizontalPadding in its style alongside the existing safe-area paddingBottom.
Preserve the current gap, alignment, and conditional flex classes while reusing
the horizontalPadding contract from the surrounding ScrollView.
---
Nitpick comments:
In `@src/app/learning-plans/`[planId]/review.tsx:
- Around line 306-309: Update the className conditional for the primary action
button so the stacked shouldStackInlineContent branch includes min-h-14 while
remaining vertically expandable, preserving the existing h-14 behavior for the
non-stacked branch.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6fa8f8cc-0cd1-4f54-b54b-af6ff0226e4d
📒 Files selected for processing (12)
src/app/(app)/home.tsxsrc/app/(app)/learning-plans.tsxsrc/app/entry/new.tsxsrc/app/learning-plans/[planId]/index.tsxsrc/app/learning-plans/[planId]/review.tsxsrc/app/notifications.tsxsrc/app/validation/overview.tsxsrc/components/screen-header.tsxsrc/components/ui/list-row.tsxsrc/features/auth/auth-content-size-layout.test.tssrc/features/auth/auth-content-size-layout.tssrc/features/auth/dayova-auth-flow.tsx
🚧 Files skipped from review as they are similar to previous changes (9)
- src/components/ui/list-row.tsx
- src/components/screen-header.tsx
- src/app/validation/overview.tsx
- src/app/learning-plans/[planId]/index.tsx
- src/app/notifications.tsx
- src/app/(app)/learning-plans.tsx
- src/app/entry/new.tsx
- src/app/(app)/home.tsx
- src/features/auth/dayova-auth-flow.tsx
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
Follow-up: iPad AXXXL range-ring fixThe earlier iPad recording exposed one remaining defect: the After — iPad (A16), portrait, iOS AXXXL: 
Regression checks: iPhone/iPad/Android default ring unchanged; Android font scale 2.0 and display density 560 pass; 140 tests, lint/typecheck, formatting, Matt Pocock standards/spec review, and CodeRabbit CLI all pass. |
|
Follow-up complete in
The PR description now contains a controlled default iPad before/after pair plus iPad AXXXL and Android combined-maximum evidence. Validation:
The PR is back in draft because DAY-190 still requires the final physical iPhone and physical Android acceptance matrix. @Gamius00 has been re-requested for the new commit. |
|
@Gamius00 Please re review the latest commits |
|
I will try to test it tomorrow on my physical device. Can you test it on your Android please? |
|
Merge-order coordination for PR #316: DAY-109 is the next intended atomic change to The current #271 head is preserved by remote tag |
2 participants
What changed
Makes Dayova's full portrait experience resilient across iOS and Android system text sizes and display/visualisation sizes, including portrait tablets, while preserving the existing/Figma composition at default sizes.
Root cause: many screens combined authored fixed heights/absolute offsets with unbounded system-scaled text, and shared containers did not provide a consistent readable-width or reflow contract.
Linear: DAY-190 · DAY-159
Before / after
iPadOS — default RangeSelector optical centering
Controlled comparison: the same iPad (A16) simulator, default iPadOS text/display settings, portrait viewport, onboarding step, selected value, and 88 px ring. Only the JavaScript bundle changes—from
ad52c5ato95e9c20.ad52c5a95e9c20Pixel-bound analysis reduced the visible text-group center error from 2.25 pt to 0.25 pt. The ring stays exactly 88 px; this is a deliberate correction to the previously buggy default text placement, not a general default-layout redesign.
RangeSelector and selected-chip extremes after
95e9c20The measured iPad AXXXL text-group center error is 0.75 pt after the fix (previously 7 pt). Android was also exercised through the fact screen, scrollable strengths list, selection interaction, and selected-state rendering rather than screenshot-only launch coverage.
iOS — iPhone 16, Accessibility Medium, Home
Controlled comparison: the same authenticated simulator, account, Home data, portrait viewport, top scroll position, and fixed 09:41 status bar. Only the JavaScript bundle changes—from
origin/main(a161766) to this PR atb2021ea.origin/mainb2021eaBefore, the notification control is cut by the right edge, the date and time collide, and the calendar row escapes both sides of its card. After, controls stay in bounds, the date and time separate cleanly, and the summary remains readable; larger content consumes more vertical space and continues by vertical scrolling instead of being compressed.
Android — display size 560, Settings
Complete size-matrix evidence
iOS — all 12 selectable text sizes, iPhone 13 mini portrait
Steps covered: XS, S, M, L, XL, XXL, XXXL, AXM, AXL, AXXL, AXXXL, AXXXXL.
Android — all 7 selectable font sizes, Pixel 9 portrait
Font scales covered: 0.85, 1.0, 1.15, 1.30, 1.50, 1.80, 2.0.
Android — all 5 selectable display sizes, Pixel 9 portrait
Display densities covered: 356, 420 (default), 460, 500, 540.
Android — 360×640dp small portrait, font scale 2.0
Android — portrait-tablet compatibility, default versus combined maximum sizing
Android — empty validation and selection-clears-error at combined maximum sizing
Screen recordings
Android Pixel 9 — font scale 2.0 + display density 540
Full-timeline observations: auth landing and footer scrolling; registration; all intro screens; range question and CTA; fact-card scrolling and CTA; long choice list; empty validation. Selection-clears-error is shown separately in the comparison above.
Coverage: full 0.0–46.5s timeline sampled every 2.5s (19 frames), with the 43.0–46.4s validation interval inspected at sub-1s/1s detail; 1080×2424 H.264 source; no audio stream. The uploaded asset is a 360×808, 46.5-second animated rendering of the same complete timeline.
iOS iPad AXXXL — full 96.1-second timeline
iOS iPad AXXXL — condensed highlight
Updated full-timeline observations at
95e9c20: 0–24s auth choice; 25–48s intro 1; 49–70s intro 2; 71–92s intro 3; 93–96.1s range question. The final state now shows30 mincompact and optically centered inside the circular progress ring, with no clipping or overflow.Coverage of the uploaded full GIF: 96.10-second timeline; 96 full-timeline frames sampled at 1 fps across 6 contact sheets, plus 16 focused frames from 92.0–96.1s at 4 fps; no audio stream. Coverage of the uploaded condensed GIF: 8.01-second timeline; 16 full-timeline frames sampled at 2 fps, plus 12 focused frames from 5.0–8.0s at 4 fps; no audio stream. Every generated contact sheet was inspected.
Validation
pnpm checkpnpm test— 33 files, 142 testspnpm format:checkgit diff --checkpnpm check:unused— only the unchangedmainbaseline theme exports remain1eeb6ae— tablet handle centering, tablet sheet centering, sticky-footer alignment, creation-loader scrolling, and enlarged/compact range-carousel sizing while preserving default geometry1eeb6ae: all three inline threads confirmed addressed and resolved; no new findings0.6.5: full agent review of the committed branch againstmain; 41 changed files reviewed, 0 findings (coderabbit doctor: 9 passed, 0 warnings, 0 failed)15fec9e: 12 changed files reviewed; two minor suggestions were rejected after direct comparison withmainbecause they would alter the required default header composition15fec9e: responsive review-footer padding, stacked minimum touch height, and overclaimed OTP test wording were corrected incdbe40c; the inline thread is resolvedb2021eanow omits that override entirely in default mode; final Standards and Spec reviews are cleancdbe40candb2021ea: completed with no new findings; all checks greenad52c5a: exact iPad AXXXL after evidence plus Android default, font scale 2.0, and display density 560 passes; default 88 px geometry unchanged0.6.5follow-up review of the two-file delta: 0 issues95e9c20: controlled iPad default before/after, iPad AXXXL, Android combined-maximum range, and Android combined-maximum selected-state evidence above0.6.5final uncommitted review: 5 changed files, 0 issuespnpm check,pnpm format:check,git diff --check, and 33 files / 142 tests all passDraft gate
No physical iPhone or Android device was connected. Per the agreed quality gate, this PR stays draft until both physical-device passes are completed and recorded. Gamius00's approval is already recorded. iOS Display Zoom is not exposed by the current simulator, so that remains part of the physical iPhone pass. Dedicated tablet composition, landscape, split-screen, foldables, desktop, and Windows remain in the linked follow-up Linear work rather than expanding this change.
Summary by CodeRabbit