fix(filters): make sort sheet options dense like the other sheets - #516
Merged
Conversation
SortOptionsSheet built its options with a plain ListTile and omitted dense: true, while the category, style, and visibility sheets all pass it on their CheckboxListTiles. With no ListTileTheme in lib/, the sort tile fell back to the Material 3 default of styling its title from bodyLarge (~16sp), giving a 48px row pitch against 40px elsewhere — so the sort picker read as a different, larger font. Add dense: true so the row pitch and title size match. Cosmetic only; the Semantics labels and the widget tree shape are unchanged. The issue also asked whether controlAffinity wants aligning here. It does not — ListTile has no controlAffinity parameter, and passing leading: Radio already is leading affinity, matching the other sheets. Fixes #507 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01A5nhJiJuRjtP3q7mTrsXBt
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes an inconsistency in the filter bottom sheets by making SortOptionsSheet option rows use the same dense row styling as the category/style/visibility sheets, addressing the “Sort By sheet options render larger” regression (#507).
Changes:
- Set
dense: trueon theListTilerows used bySortOptionsSheet. - Add a widget regression test that verifies the sort sheet rows are dense and match the rendered row height of an already-dense category sheet row.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| lib/widgets/drink_filter_sheets.dart | Makes SortOptionsSheet option rows dense to match other filter sheets’ row density. |
| test/widgets/drink_filter_sheets_test.dart | Adds a regression test covering both the dense flag and the rendered row height parity with a dense checkbox row. |
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://claude-ship-issues-507-0t88t.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.
Summary
SortOptionsSheetbuilt its options with a plainListTileand omitteddense: true, while the category, style, and visibility sheets in the same file all pass it on theirCheckboxListTiles. With noListTileThemeanywhere inlib/, the sort tile fell back to the Material 3 default of styling its title frombodyLarge(~16sp) — a 48px row pitch against 40px everywhere else. The result read as a different, larger font in the sort picker.Adding
dense: truebrings it in line.Changes
lib/widgets/drink_filter_sheets.dartdense: trueon theListTileinSortOptionsSheettest/widgets/drink_filter_sheets_test.dartOn the
controlAffinityquestion in the issueThe issue asked whether the radio-vs-checkbox
controlAffinityalso wants aligning. It does not:ListTilehas nocontrolAffinityparameter, and passingleading: Radio<DrinkSort>(...)already is leading affinity — consistent with the explicitListTileControlAffinity.leadingon the other sheets. No change made.Testing
The new test asserts two things: that every sort-option
ListTileis dense, and that the rendered height of a sort row equals the rendered height of a category row. The second assertion is deliberately self-calibrating against a sheet that was always dense rather than hardcoding a pixel constant, so it stays honest if Material changes its metrics.Verified the test is a real guard — with the one-line fix stashed it fails, and passes with it restored:
./bin/mise run checkis green: 1314 tests pass (baseline 1313), analyzer clean, formatter clean.Scope notes
test/goldens/covers the brewery, drink-detail, and style screens only, no filter sheets. No golden regeneration was run.Semanticswrapper and labels untouched. The issue notes this is cosmetic with no accessibility defect, and that remains true.Not verifiable by an agent: the visual result in a real browser or on device. Worth a glance at the sort sheet next to the style sheet on the Pages preview.
Fixes #507
Generated by Claude Code