Skip to content

fix(announcements): make the home banner fit its text - #2683

Merged
ethicnology merged 3 commits into
mainfrom
fix/announcement-card-height
Aug 17, 2026
Merged

fix(announcements): make the home banner fit its text#2683
ethicnology merged 3 commits into
mainfrom
fix/announcement-card-height

Conversation

@wired-pasteque

Copy link
Copy Markdown
Collaborator

Before the banner had a hardcoded height, so it never matched the text it showed. Now the carousel sizes itself to its tallest card (no fixed height).

Simulator Screenshot - iPhone 17e - 2026-08-17 at 14 29 49

@wired-pasteque wired-pasteque self-assigned this Aug 17, 2026
@claude

This comment was marked as outdated.

@wired-pasteque
wired-pasteque changed the base branch from develop to main August 17, 2026 19:06
@wired-pasteque

Copy link
Copy Markdown
Collaborator Author

@claude, review the changes

@claude

claude Bot commented Aug 17, 2026

Copy link
Copy Markdown

Claude finished @wired-pasteque's task in 6m 10s —— View job


Code review — PR #2683: make the home banner fit its text

  • Gather context (AGENTS.md, diff vs base, related files)
  • Review announcement_card.dart changes
  • Review announcement_carousel.dart changes
  • Review bull_ui.dart export changes
  • Review test changes
  • Post review feedback

Verdict

Solid change — the approach is correct and well-tested. PageView fundamentally can't size itself to its children, so replacing it with a horizontal SingleChildScrollView + PageScrollPhysics + IntrinsicHeight row is the right way to get "carousel as tall as its tallest card". The test rewrite is a real improvement: the harness now mirrors the actual home-screen usage (SliverToBoxAdapter inside a CustomScrollView, matching wallet_home_screen.dart:121), and it covers text scale, narrow screens, multi-card height sharing, page snapping, and window resize. The opaque background via Color.alphaBlend with colors.background stays theme-token-compliant (AGENTS.md rule #10) and is pinned by a test. No architecture-rule violations; the widget-state-in-StatefulWidget split follows rule #5.

Found no correctness bugs. A few observations, roughly in order of importance:

1. didChangeMetrics realigns on every metrics change, not just width changes (minor)

lib/features/announcements/ui/widgets/announcement_carousel.dart:100-112didChangeMetrics fires for any window-metric change: keyboard insets appearing on a route above, system-UI/inset changes, display changes — not only width changes. Each firing schedules a post-frame jumpTo, which goes through goIdle, so it cancels an in-flight drag or snap animation and hard-jumps to the page boundary even when the width (and therefore the page geometry) is unchanged. On the home screen this is a rare, low-impact glitch, but the guard is cheap: remember the last viewportDimension and only jumpTo when it actually changed. Fix this →

2. Trade-off worth knowing: PageController gave resize-preservation for free

The old PageView + PageController preserved the current page across viewport resizes automatically (PageController.keepPage stores the page fraction, not pixels). Switching to a raw ScrollController loses that, and the didChangeMetrics observer is a manual reimplementation of it. It's done correctly (the target page is captured before the new layout, the jump is clamped to maxScrollExtent, and the resize test at announcement_carousel_test.dart:246 pins the behavior) — but it only covers window-metric changes. A width change that arrives through parent layout without a window resize wouldn't realign. That can't happen today (the carousel's horizontal padding is constant and it spans the screen), so this is fine as-is; just flagging the assumption in case the carousel is ever embedded differently.

3. Small-scale assumptions that are fine today

  • All cards are now built eagerly (the old PageView.builder was lazy), and IntrinsicHeight adds an extra layout pass that nests with BullInfoCard's own internal IntrinsicHeight. Both are inherent to the tallest-card design and harmless at the current announcement count (a handful) — just don't let this carousel grow into a long feed.
  • Dot updates now happen at the 50% swipe point (.round() in _onScroll, announcement_carousel.dart:119) instead of on page settle like the old onPageChanged. Arguably nicer; behavior change is intentional-looking and covered by the snap test.

4. bull_ui barrel now exports binding types (nit)

packages/bull_ui/lib/bull_ui.dartWidgetsBinding / WidgetsBindingObserver are framework binding/scheduler types, not UI-kit widgets, so this slightly widens the design-system surface beyond "widgets". It's consistent with the repo policy that kit-based features import only package:bull_ui/bull_ui.dart (and this PR uses it to drop the previous flutter/widgets.dart show MediaQuery import, a net win), so no change requested — just making sure the widening is deliberate. PageView/PageController stay correctly exported (still used by the wizard feature).

5. Test-coverage suggestion (optional)

One behavior the code handles but no test pins: dismissing the currently visible last card of two or more. The scroll offset then exceeds the new maxScrollExtent (physics spring it back) and the dot index relies on the activePage clamp at announcement_carousel.dart:154. A small regression test would protect that clamp if someone later "simplifies" it away.

Also, for anyone reading the exact-equality assertions on position.pixels (e.g. announcement_carousel_test.dart:222): they're safe, not flaky — ScrollSpringSimulation returns exactly its end value once done, and jumpTo sets exact pixels.

CI / local checks

I couldn't run make analyze / make unit-test in this sandbox (the commands aren't allowlisted for this workflow — add them to --allowedTools if you want me to run them next time). The Analyze and Test run was still in progress at review time; it runs the same checks, so treat its result as the source of truth.


Summary: No blocking issues. The one change I'd actually make is the width guard in didChangeMetrics (item 1); items 2–5 are observations and optional hardening.

@ethicnology
ethicnology force-pushed the fix/announcement-card-height branch from 8ae3e20 to 1316a31 Compare August 17, 2026 20:04
@ethicnology
ethicnology merged commit ed294dc into main Aug 17, 2026
1 of 2 checks passed
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