refactor(widgets): share festival status badge and sheet handle - #501
Merged
Conversation
Fold FestivalCard._buildStatusBadge into FestivalStatusBadge behind a compact flag so both surfaces keep their current labels and geometry from one source of truth. Promote the filter sheets' private _SheetHandle to a shared SheetHandle and use it for the three hand-rolled festival/settings/theme sheet handles. Fixes #499
Replace the constructor that accepted `key` and forwarded it to the inner Container while passing null to super. Swallowing `key` made SheetHandle(key: k) silently unkeyed, which is surprising for a shared widget. An explicit `handleKey` keeps the three drag-handle test assertions matching a single Container and leaves `key` behaving normally. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017B31wKKL578bV18hcpXyE2
Contributor
There was a problem hiding this comment.
Pull request overview
Refactors widget duplication by centralizing the festival status badge logic and the modal-sheet drag handle into shared widgets, keeping existing user-visible copy on both surfaces (compact app-bar vs long card labels).
Changes:
- Deduplicates festival status badge implementation via
FestivalStatusBadge(compact: ...), keeping both label sets and consolidating colors/labels in_styleFor. - Promotes the bottom-sheet drag handle into a shared
SheetHandlewidget and replaces hand-rolled handles across sheets. - Updates/extends widget tests to lock in the compact vs long label behavior and the shared-handle geometry/keying behavior.
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| test/widgets/sheet_handle_test.dart | Adds tests for SheetHandle geometry, centering, theme color, and key forwarding (noting a few brittle finders). |
| test/widgets/festival_menu_sheets_test.dart | Adds a regression assertion that festival cards use the shared FestivalStatusBadge. |
| test/widgets/festival_header_test.dart | Expands coverage to assert compact/long/default labels for all FestivalStatus values. |
| lib/widgets/widgets.dart | Exports sheet_handle.dart from the widgets barrel. |
| lib/widgets/sheet_handle.dart | Introduces the shared SheetHandle widget with explicit handleKey forwarding. |
| lib/widgets/festival_menu_sheets.dart | Replaces bespoke status badge + hand-rolled sheet handles with shared widgets. |
| lib/widgets/festival_header.dart | Adds compact mode to FestivalStatusBadge, updates header call site, and consolidates styling via _styleFor. |
| lib/widgets/drink_filter_sheets.dart | Removes the private _SheetHandle and switches filter sheets to the shared SheetHandle. |
Comments suppressed due to low confidence (2)
test/widgets/sheet_handle_test.dart:30
- This ancestor lookup keys off find.byType(Container), which may match multiple Containers over time. Scoping it to the Container that belongs to SheetHandle makes the test less fragile.
of: find.byType(Container),
test/widgets/sheet_handle_test.dart:81
- As above, find.byType(Container) is prone to becoming ambiguous if additional Container widgets appear in the tree. Scope the finder to a descendant of SheetHandle and assert a single match.
final container = tester.widget<Container>(find.byType(Container));
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Contributor
🚀 Cloudflare Pages PreviewYour preview deployment is ready! Preview URL: https://fix-499-dedupe-festival-stat.staging-cambeerfestival.pages.dev This preview will be automatically updated when you push new commits to this PR. |
find.byType(Container) matched the whole pumped tree, so the assertions would become ambiguous if the test scaffolding ever gained a Container of its own. Scope to SheetHandle's descendants and assert a single match before reading the widget. Addresses review feedback on #501. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017B31wKKL578bV18hcpXyE2
Contributor
🚀 Cloudflare Pages PreviewYour preview deployment is ready! Preview URL: https://fix-499-dedupe-festival-stat.staging-cambeerfestival.pages.dev This preview will be automatically updated when you push new commits to this PR. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #499
The festival status badge was implemented twice with byte-identical colours but divergent user-visible labels, and
festival_menu_sheets.dartdid not importfestival_header.dartat all.Decision: keep both label sets
FestivalStatusBadgegains acompactflag (defaultfalse) that drives both wording and geometry, so neither surface changes what the user sees:compact: true(app bar)compact: false(festival cards)_styleForis now the single source of truth, returning(compactLabel, longLabel, spokenLabel, light, dark). The publicspokenLabel()API and its exact spoken values (live now,starting soon,most recent,past) are unchanged.Changes
FestivalCard._buildStatusBadgedeleted, including theBuilderit used purely to obtain acontextthatFestivalCard.buildalready had. Itsmargin: right 8became aSizedBox(width: 8)at the call site, so card spacing is unchanged._SheetHandleis promoted to a sharedSheetHandlein its own file and exported fromwidgets.dart. The three hand-rolled 32×4 drag-handle containers infestival_menu_sheets.dartnow use it, as do the four existing call sites indrink_filter_sheets.dart.SheetHandletakes an explicithandleKeythat lands on the innerContainer. Forwarding the widget's ownkeythere instead would makefind.byKeymatch two widgets and break the existingtester.widget<Container>(...)assertions withBad state: Too many elements; a separate named parameter keepskeybehaving normally for a shared widget.Testing
./bin/mise run checkgreen (analyzer clean, full suite passing). Also verified green on the union with #500, which touches the samewidgets.dartbarrel.festival_header_test.dart— compact vs default-long labels across all fourFestivalStatusvalues; existingFestivalHeadersemantics coverage untouchedfestival_menu_sheets_test.dart— the existingCOMING SOON/MOST RECENTand three drag-handle-key assertions pass unedited, which is the regression guard for the copy decision; oneFestivalStatusBadgewidget-type assertion addedsheet_handle_test.dart(new) — geometry, centring,handleKeyforwarding to a singleContainer, normalkeybehaviour, theme colourNot verified: on-device/browser rendering. The badge geometry is asserted in tests but has not been eyeballed on a real device.
Generated by Claude Code