From fe7ba8abd3febfb5570e81dd25ce87ee06206da0 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 27 Jul 2026 20:40:36 +0000 Subject: [PATCH 1/6] feat(drinks): support multi-select category filtering Category filtering was single-select (String?), so users could not view beer + international-beer + low-no together even though those three categories make up 76% of a typical festival. Switch DrinkFilterController/Service and BeerProvider to a Set-based category filter (OR logic across categories, matching the existing style-filter pattern) and convert CategoryFilterSheet from single-select radios to multi-select checkboxes that stay open across taps. Toggling a category now prunes (rather than clears) the selected styles, keeping only styles still present under the new category scope, since an unconditional clear was too destructive once several categories can be active at once. Analytics keeps logCategoryFilter's existing String? signature; the provider now passes a canonical sorted, comma-joined value (or null when cleared) so the logged value is independent of selection order. Fixes #319 --- .../controllers/drink_filter_controller.dart | 72 +++++--- lib/domain/services/drink_filter_service.dart | 17 +- lib/providers/beer_provider.dart | 34 +++- lib/screens/drinks_screen.dart | 21 ++- lib/widgets/drink_filter_sheets.dart | 156 ++++++++++-------- test/beer_provider_test.dart | 52 ++++-- .../drink_filter_controller_test.dart | 87 +++++++--- .../services/drink_filter_service_test.dart | 48 ++++-- test/provider_test.dart | 29 +++- test/widgets/drink_filter_sheets_test.dart | 99 +++++++++-- 10 files changed, 449 insertions(+), 166 deletions(-) diff --git a/lib/domain/controllers/drink_filter_controller.dart b/lib/domain/controllers/drink_filter_controller.dart index b2ff8a09..31f64806 100644 --- a/lib/domain/controllers/drink_filter_controller.dart +++ b/lib/domain/controllers/drink_filter_controller.dart @@ -57,7 +57,7 @@ class DrinkFilterController { List _source = []; List _filtered = []; - String? _selectedCategory; + Set _selectedCategories = {}; Set _selectedStyles = {}; DrinkSort _currentSort = DrinkSort.nameAsc; String _searchQuery = ''; @@ -67,7 +67,7 @@ class DrinkFilterController { // --- Criteria getters --- - String? get selectedCategory => _selectedCategory; + Set get selectedCategories => Set.unmodifiable(_selectedCategories); Set get selectedStyles => _selectedStyles; DrinkSort get currentSort => _currentSort; String get searchQuery => _searchQuery; @@ -86,13 +86,11 @@ class DrinkFilterController { List get filteredDrinks => _filtered; /// Unique categories present in scope (see class doc), sorted naturally. - /// The [selectedCategory], if any, is always included even if its scoped + /// Every [selectedCategories] entry is always included even if its scoped /// count is 0 (invariant 1). List get availableCategories { - final categories = _scopeFor( - _Facet.category, - ).map((d) => d.category).toSet(); - if (_selectedCategory != null) categories.add(_selectedCategory!); + final categories = _scopeFor(_Facet.category).map((d) => d.category).toSet() + ..addAll(_selectedCategories); return categories.toList()..sort(); } @@ -112,15 +110,16 @@ class DrinkFilterController { return styles.toList()..sort(StringComparisonHelper.compareCaseInsensitive); } - /// Drink count per category, scoped per the class doc. A [selectedCategory] - /// with no matches in scope is still present, mapped to 0 (invariant 1). + /// Drink count per category, scoped per the class doc. Every entry of + /// [selectedCategories] with no matches in scope is still present, mapped + /// to 0 (invariant 1). Map get categoryCountsMap { final counts = {}; for (final drink in _scopeFor(_Facet.category)) { counts[drink.category] = (counts[drink.category] ?? 0) + 1; } - if (_selectedCategory != null) { - counts.putIfAbsent(_selectedCategory!, () => 0); + for (final category in _selectedCategories) { + counts.putIfAbsent(category, () => 0); } return counts; } @@ -170,7 +169,7 @@ class DrinkFilterController { void recompute() { final filtered = _filterService.filterDrinks( _source, - category: _selectedCategory, + categories: _selectedCategories, styles: _selectedStyles, favoritesOnly: _showFavoritesOnly, visibilityFilters: _visibilityFilters, @@ -182,16 +181,49 @@ class DrinkFilterController { // --- Mutators (synchronous, no side effects) --- - /// Set the category filter. Clears any active style filter, since styles are - /// category-dependent. - void setCategory(String? category) { - _selectedCategory = category; - if (_selectedStyles.isNotEmpty) { - _selectedStyles = {}; + /// Toggle a single category in the multi-select category filter. + /// + /// Prunes (rather than clears) the style selection: a style survives the + /// toggle only if it is still present in the style facet's scope under the + /// *new* category selection (see [_scopeFor] / [availableStyles]). Under + /// single-select this used to be an unconditional clear, but that is too + /// destructive for multi-select — e.g. adding "perry" to an existing + /// "cider" selection would otherwise wipe a cider style the user just + /// picked, even though it's still relevant to the combined selection. + void toggleCategory(String category) { + if (_selectedCategories.contains(category)) { + _selectedCategories = Set.from(_selectedCategories)..remove(category); + } else { + _selectedCategories = Set.from(_selectedCategories)..add(category); } + _pruneStylesToScope(); + recompute(); + } + + /// Clear all selected categories. + void clearCategories() { + _selectedCategories = {}; + _pruneStylesToScope(); recompute(); } + /// Drop any selected style no longer present in the style facet's current + /// scope (i.e. under the just-changed category selection). Recomputes + /// scope directly against `_selectedStyles` rather than [availableStyles] + /// so it isn't affected by that getter's own invariant-1 re-inclusion of + /// already-selected styles. + void _pruneStylesToScope() { + if (_selectedStyles.isEmpty) return; + final scopedStyles = _scopeFor(_Facet.style) + .where((d) => d.style != null && d.style!.isNotEmpty) + .map((d) => d.style!) + .toSet(); + final pruned = _selectedStyles.where(scopedStyles.contains).toSet(); + if (pruned.length != _selectedStyles.length) { + _selectedStyles = pruned; + } + } + /// Toggle a single style in the multi-select style filter. void toggleStyle(String style) { if (_selectedStyles.contains(style)) { @@ -265,7 +297,7 @@ class DrinkFilterController { /// festivals). Sort, visibility, and allergen preferences are intentionally /// preserved. void clearCategoryStyleSearch() { - _selectedCategory = null; + _selectedCategories = {}; _selectedStyles = {}; _searchQuery = ''; recompute(); @@ -291,7 +323,7 @@ class DrinkFilterController { /// applied here (see class doc). Iterable _scopeFor(_Facet facet) => _filterService.filterDrinks( _source, - category: facet == _Facet.category ? null : _selectedCategory, + categories: facet == _Facet.category ? const {} : _selectedCategories, styles: facet == _Facet.style ? const {} : _selectedStyles, favoritesOnly: _showFavoritesOnly, visibilityFilters: _visibilityFilters, diff --git a/lib/domain/services/drink_filter_service.dart b/lib/domain/services/drink_filter_service.dart index 21f143da..1ce00ed9 100644 --- a/lib/domain/services/drink_filter_service.dart +++ b/lib/domain/services/drink_filter_service.dart @@ -10,13 +10,16 @@ class DrinkFilterService { /// Shared source of truth for which fields free-text search covers. static const SearchMatchService _searchMatcher = SearchMatchService(); - /// Filter drinks by category + /// Filter drinks by categories (multi-select with OR logic) /// - /// Returns all drinks if [category] is null + /// Returns all drinks if [categories] is empty /// Uses lazy evaluation - call .toList() to materialize - Iterable filterByCategory(Iterable drinks, String? category) { - if (category == null) return drinks; - return drinks.where((d) => d.category == category); + Iterable filterByCategories( + Iterable drinks, + Set categories, + ) { + if (categories.isEmpty) return drinks; + return drinks.where((d) => categories.contains(d.category)); } /// Filter drinks by styles (multi-select with OR logic) @@ -138,14 +141,14 @@ class DrinkFilterService { /// chain materialises once at the end. List filterDrinks( List drinks, { - String? category, + Set? categories, Set? styles, bool favoritesOnly = false, Set visibilityFilters = const {}, Set excludedAllergens = const {}, String searchQuery = '', }) { - Iterable result = filterByCategory(drinks, category); + Iterable result = filterByCategories(drinks, categories ?? const {}); result = filterByStyles(result, styles ?? const {}); result = filterByFavorites(result, favoritesOnly: favoritesOnly); result = filterByAvailability( diff --git a/lib/providers/beer_provider.dart b/lib/providers/beer_provider.dart index 57fd3879..3853a483 100644 --- a/lib/providers/beer_provider.dart +++ b/lib/providers/beer_provider.dart @@ -118,7 +118,7 @@ class BeerProvider extends ChangeNotifier { /// Non-null when a background refresh failed but cached drinks remain shown. String? get refreshNotice => _refreshNotice; String? get festivalsError => _festivalsError; - String? get selectedCategory => _filter.selectedCategory; + Set get selectedCategories => _filter.selectedCategories; Set get selectedStyles => _filter.selectedStyles; DrinkSort get currentSort => _filter.currentSort; String get searchQuery => _filter.searchQuery; @@ -657,16 +657,34 @@ class BeerProvider extends ChangeNotifier { } } - /// Set category filter - void setCategory(String? category) { - // The controller clears the style filter when the category changes, since - // styles are category-dependent. - _filter.setCategory(category); + /// Toggle a category filter (supports multiple category selection). + /// + /// The controller prunes any selected style no longer in scope under the + /// new category selection, since styles are category-dependent. + void toggleCategory(String category) { + _filter.toggleCategory(category); notifyListeners(); - // Log analytics event (fire and forget) - unawaited(_analyticsService.logCategoryFilter(category)); + // Log analytics event (fire and forget). Canonical value: null when the + // selection is empty, otherwise the selected categories sorted and + // joined with ',' — logCategoryFilter's signature is unchanged, so this + // keeps a single, order-independent value per selection. + unawaited(_analyticsService.logCategoryFilter(_canonicalCategoryFilter)); + } + + /// Clear all category filters. + void clearCategories() { + _filter.clearCategories(); + notifyListeners(); + unawaited(_analyticsService.logCategoryFilter(_canonicalCategoryFilter)); } + /// Canonical analytics value for the current category selection: `null` + /// when empty, otherwise the selected categories sorted and joined with + /// ',' so the logged value doesn't depend on selection order. + String? get _canonicalCategoryFilter => _filter.selectedCategories.isEmpty + ? null + : (_filter.selectedCategories.toList()..sort()).join(','); + /// Toggle a style filter (supports multiple style selection) void toggleStyle(String style) { _filter.toggleStyle(style); diff --git a/lib/screens/drinks_screen.dart b/lib/screens/drinks_screen.dart index 785446f3..930ff9ef 100644 --- a/lib/screens/drinks_screen.dart +++ b/lib/screens/drinks_screen.dart @@ -143,6 +143,13 @@ class _DrinksScreenState extends State { : provider.selectedStyles.length == 1 ? provider.selectedStyles.first : '${provider.selectedStyles.length} styles'; + final categoryLabel = provider.selectedCategories.isEmpty + ? 'Category' + : provider.selectedCategories.length == 1 + ? BeverageTypeHelper.formatBeverageType( + provider.selectedCategories.first, + ) + : '${provider.selectedCategories.length} categories'; return Container( padding: const EdgeInsets.symmetric(horizontal: 12, vertical: 6), @@ -150,13 +157,13 @@ class _DrinksScreenState extends State { children: [ Expanded( child: FilterButton( - label: provider.selectedCategory ?? 'Category', - semanticLabel: provider.selectedCategory != null - ? 'Filter by category: ${provider.selectedCategory}' - : 'Filter by category', + label: categoryLabel, + semanticLabel: provider.selectedCategories.isEmpty + ? 'Filter by category' + : 'Filter by category: ${provider.selectedCategories.join(', ')}', icon: Icons.filter_list, onPressed: () => showCategoryFilter(context), - isActive: provider.selectedCategory != null, + isActive: provider.selectedCategories.isNotEmpty, ), ), if (hasStyleFilter) ...[ @@ -344,7 +351,7 @@ class _DrinksScreenState extends State { ), const SizedBox(height: 8), const Text('Try adjusting your filters'), - if (provider.selectedCategory != null) ...[ + if (provider.selectedCategories.isNotEmpty) ...[ const SizedBox(height: 16), Semantics( label: 'Clear category filter', @@ -352,7 +359,7 @@ class _DrinksScreenState extends State { button: true, excludeSemantics: true, child: OutlinedButton( - onPressed: () => provider.setCategory(null), + onPressed: () => provider.clearCategories(), child: const Text('Clear Filters'), ), ), diff --git a/lib/widgets/drink_filter_sheets.dart b/lib/widgets/drink_filter_sheets.dart index 84b3dae0..2e791d99 100644 --- a/lib/widgets/drink_filter_sheets.dart +++ b/lib/widgets/drink_filter_sheets.dart @@ -5,11 +5,9 @@ import '../providers/providers.dart'; import '../utils/utils.dart'; import 'sheet_handle.dart'; -/// Shows the category filter as a modal bottom sheet. -void showCategoryFilter(BuildContext context) { - final provider = context.read(); - _showSheet(context, (_) => CategoryFilterSheet(provider: provider)); -} +/// Shows the category filter (multi-select) as a modal bottom sheet. +void showCategoryFilter(BuildContext context) => + _showSheet(context, (_) => const CategoryFilterSheet()); /// Shows the style filter (multi-select) as a modal bottom sheet. void showStyleFilter(BuildContext context) => @@ -33,84 +31,112 @@ void _showSheet(BuildContext context, WidgetBuilder builder) { ); } -/// Single-select category filter sheet. +/// Category filter sheet with checkboxes for multi-select. Categories arrive +/// already sorted naturally from [BeerProvider.availableCategories]. class CategoryFilterSheet extends StatelessWidget { - final BeerProvider provider; - - const CategoryFilterSheet({required this.provider, super.key}); + const CategoryFilterSheet({super.key}); @override Widget build(BuildContext context) { final theme = Theme.of(context); - final categories = provider.availableCategories; - final counts = provider.categoryCountsMap; - return Container( - padding: const EdgeInsets.all(16), - constraints: BoxConstraints( - maxHeight: MediaQuery.of(context).size.height * 0.7, - ), - child: Column( - mainAxisSize: MainAxisSize.min, - crossAxisAlignment: CrossAxisAlignment.start, - children: [ - const SheetHandle(), - const SizedBox(height: 16), - Text('Filter by Category', style: theme.textTheme.titleLarge), - const SizedBox(height: 16), - Flexible( - child: SingleChildScrollView( - child: RadioGroup( - groupValue: provider.selectedCategory, - onChanged: (value) { - provider.setCategory(value); - Navigator.pop(context); - }, - child: Column( - mainAxisSize: MainAxisSize.min, - children: [ + return Consumer( + builder: (context, beerProvider, child) { + final categories = beerProvider.availableCategories; + final counts = beerProvider.categoryCountsMap; + final selectedCategories = beerProvider.selectedCategories; + + return Container( + padding: const EdgeInsets.all(16), + constraints: BoxConstraints( + maxHeight: MediaQuery.of(context).size.height * 0.7, + ), + child: Column( + mainAxisSize: MainAxisSize.min, + crossAxisAlignment: CrossAxisAlignment.start, + children: [ + const SheetHandle(), + const SizedBox(height: 16), + Row( + mainAxisAlignment: MainAxisAlignment.spaceBetween, + children: [ + Text('Filter by Category', style: theme.textTheme.titleLarge), + if (selectedCategories.isNotEmpty) Semantics( - label: - 'Show all drinks, ${provider.allDrinks.length} total', - selected: provider.selectedCategory == null, + label: 'Clear all category filters', + hint: 'Double tap to remove all category filters', button: true, excludeSemantics: true, - child: ListTile( - leading: const Radio(value: null), - title: Text('All (${provider.allDrinks.length})'), - onTap: () { - provider.setCategory(null); - Navigator.pop(context); + child: TextButton.icon( + icon: const Icon(Icons.clear, size: 18), + label: const Text('Clear'), + onPressed: () { + beerProvider.clearCategories(); }, + style: TextButton.styleFrom( + visualDensity: VisualDensity.compact, + ), ), ), - ...categories.map((category) { - final formattedCategory = - BeverageTypeHelper.formatBeverageType(category); - final count = counts[category] ?? 0; - return Semantics( - label: 'Filter by $formattedCategory, $count drinks', - selected: provider.selectedCategory == category, + ], + ), + const SizedBox(height: 16), + Flexible( + child: SingleChildScrollView( + child: Column( + mainAxisSize: MainAxisSize.min, + children: [ + Semantics( + label: + 'Show all drinks, ${beerProvider.allDrinks.length} total', + value: selectedCategories.isEmpty + ? 'Selected' + : 'Not selected', + selected: selectedCategories.isEmpty, button: true, excludeSemantics: true, - child: ListTile( - leading: Radio(value: category), - title: Text('$formattedCategory ($count)'), - onTap: () { - provider.setCategory(category); - Navigator.pop(context); - }, + child: CheckboxListTile( + key: const ValueKey('category-all'), + value: selectedCategories.isEmpty, + onChanged: (_) => beerProvider.clearCategories(), + title: Text('All (${beerProvider.allDrinks.length})'), + controlAffinity: ListTileControlAffinity.leading, + dense: true, ), - ); - }), - ], + ), + ...categories.map((category) { + final formattedCategory = + BeverageTypeHelper.formatBeverageType(category); + final count = counts[category] ?? 0; + final isSelected = selectedCategories.contains( + category, + ); + return Semantics( + label: 'Filter by $formattedCategory, $count drinks', + value: isSelected ? 'Selected' : 'Not selected', + selected: isSelected, + button: true, + excludeSemantics: true, + child: CheckboxListTile( + key: ValueKey('category-$category'), + value: isSelected, + onChanged: (_) => + beerProvider.toggleCategory(category), + title: Text('$formattedCategory ($count)'), + controlAffinity: ListTileControlAffinity.leading, + dense: true, + ), + ); + }), + ], + ), ), ), - ), + const SizedBox(height: 16), + ], ), - const SizedBox(height: 16), - ], - ), + ); + }, ); } } diff --git a/test/beer_provider_test.dart b/test/beer_provider_test.dart index a01a46cb..72ae5d94 100644 --- a/test/beer_provider_test.dart +++ b/test/beer_provider_test.dart @@ -217,7 +217,7 @@ void main() { }); group('category filter', () { - test('setCategory filters drinks by category', () async { + test('toggleCategory filters drinks by category', () async { provider = BeerProvider( drinkRepository: mockDrinkRepository, festivalRepository: mockFestivalRepository, @@ -231,13 +231,13 @@ void main() { ).thenAnswer((_) async => sampleDrinks); await provider.loadDrinks(); - provider.setCategory('beer'); + provider.toggleCategory('beer'); expect(provider.drinks.length, 2); expect(provider.drinks.every((d) => d.category == 'beer'), isTrue); }); - test('setCategory with null shows all drinks', () async { + test('toggling on two categories shows drinks from both', () async { provider = BeerProvider( drinkRepository: mockDrinkRepository, festivalRepository: mockFestivalRepository, @@ -251,14 +251,42 @@ void main() { ).thenAnswer((_) async => sampleDrinks); await provider.loadDrinks(); - provider.setCategory('beer'); + provider + ..toggleCategory('beer') + ..toggleCategory('cider'); + + expect(provider.selectedCategories, {'beer', 'cider'}); + expect(provider.drinks.length, 4); + expect( + provider.drinks.map((d) => d.name), + containsAll(['Alpha Ale', 'Beta Bitter', 'Crisp Cider']), + ); + }); + + test('clearCategories shows all drinks', () async { + provider = BeerProvider( + drinkRepository: mockDrinkRepository, + festivalRepository: mockFestivalRepository, + analyticsService: mockAnalyticsService, + ); + await provider.initialize(); + + final sampleDrinks = createSampleDrinks(); + when( + mockDrinkRepository.getDrinks(any), + ).thenAnswer((_) async => sampleDrinks); + await provider.loadDrinks(); + + provider.toggleCategory('beer'); expect(provider.drinks.length, 2); - provider.setCategory(null); + provider.clearCategories(); expect(provider.drinks.length, 4); + expect(provider.selectedCategories, isEmpty); }); - test('setCategory clears style filter', () async { + test('toggleCategory prunes a style no longer in the new category ' + 'scope', () async { provider = BeerProvider( drinkRepository: mockDrinkRepository, festivalRepository: mockFestivalRepository, @@ -275,7 +303,9 @@ void main() { provider.toggleStyle('IPA'); expect(provider.selectedStyles, contains('IPA')); - provider.setCategory('cider'); + // IPA (a beer style) is not present in cider, so restricting to + // cider prunes it from the selection. + provider.toggleCategory('cider'); expect(provider.selectedStyles, isEmpty); }); @@ -421,7 +451,7 @@ void main() { ).thenAnswer((_) async => sampleDrinks); await provider.loadDrinks(); - provider.setCategory('beer'); + provider.toggleCategory('beer'); final beerStyles = provider.availableStyles; expect(beerStyles, containsAll(['IPA', 'Bitter'])); expect(beerStyles, isNot(contains('Dry'))); @@ -1837,7 +1867,7 @@ void main() { await provider.loadDrinks(); provider - ..setCategory('beer') + ..toggleCategory('beer') ..toggleStyle('IPA'); expect(provider.drinks.length, 1); @@ -1859,7 +1889,7 @@ void main() { await provider.loadDrinks(); provider - ..setCategory('beer') + ..toggleCategory('beer') ..setSearchQuery('alpha'); expect(provider.drinks.length, 1); @@ -2895,7 +2925,7 @@ void main() { containsAll(['IPA', 'Bitter', 'Dry', 'Sweet']), ); - provider.setCategory('cider'); + provider.toggleCategory('cider'); expect(provider.styleCountsMap.keys, containsAll(['Dry', 'Sweet'])); expect(provider.styleCountsMap.containsKey('IPA'), isFalse); }); diff --git a/test/domain/controllers/drink_filter_controller_test.dart b/test/domain/controllers/drink_filter_controller_test.dart index 3995483b..269e739d 100644 --- a/test/domain/controllers/drink_filter_controller_test.dart +++ b/test/domain/controllers/drink_filter_controller_test.dart @@ -73,7 +73,7 @@ void main() { group('initial state', () { test('starts empty with default sort and no criteria', () { expect(controller.filteredDrinks, isEmpty); - expect(controller.selectedCategory, isNull); + expect(controller.selectedCategories, isEmpty); expect(controller.selectedStyles, isEmpty); expect(controller.currentSort, DrinkSort.nameAsc); expect(controller.searchQuery, isEmpty); @@ -118,30 +118,77 @@ void main() { test('narrows filtered drinks to the selected category', () { controller ..setSource(_sampleDrinks()) - ..setCategory('cider'); + ..toggleCategory('cider'); expect(controller.filteredDrinks.map((d) => d.name), [ 'Crisp Cider', 'Zesty Zider', ]); }); - test('setting category clears any active style filter', () { + test('toggleCategory adds then removes a category', () { controller ..setSource(_sampleDrinks()) - ..setCategory('beer') - ..toggleStyle('IPA'); + ..toggleCategory('beer'); + expect(controller.selectedCategories, {'beer'}); + expect(controller.filteredDrinks, hasLength(2)); + + controller.toggleCategory('beer'); + expect(controller.selectedCategories, isEmpty); + expect(controller.filteredDrinks, hasLength(4)); + }); + + test('multiple categories use OR logic', () { + controller + ..setSource(_sampleDrinks()) + ..toggleCategory('cider') + ..toggleCategory('beer'); + expect(controller.selectedCategories, {'cider', 'beer'}); + expect(controller.filteredDrinks, hasLength(4)); + }); + + test('clearCategories restores all categories', () { + controller + ..setSource(_sampleDrinks()) + ..toggleCategory('beer') + ..clearCategories(); + expect(controller.selectedCategories, isEmpty); + expect(controller.filteredDrinks, hasLength(4)); + }); + + test('toggling a category prunes a style that no longer matches the ' + 'new scope', () { + controller + ..setSource(_sampleDrinks()) + ..toggleCategory('beer') + ..toggleStyle('IPA'); // IPA only exists on a beer drink. + expect(controller.selectedStyles, {'IPA'}); + + // Adding cider alongside beer keeps IPA in scope (beer is still + // selected) — this is the "don't wipe the whole selection" case. + controller.toggleCategory('cider'); expect(controller.selectedStyles, {'IPA'}); - controller.setCategory('cider'); + // Removing beer leaves only cider selected; IPA no longer matches + // any drink in scope, so it is pruned. + controller.toggleCategory('beer'); expect(controller.selectedStyles, isEmpty); }); - test('null category shows all categories again', () { + test('toggling a category keeps a style that still matches the new ' + 'scope', () { controller ..setSource(_sampleDrinks()) - ..setCategory('beer') - ..setCategory(null); - expect(controller.filteredDrinks, hasLength(4)); + ..toggleCategory('cider') + ..toggleStyle('Dry'); // Dry only exists on a cider drink. + expect(controller.selectedStyles, {'Dry'}); + + // Adding beer alongside cider: Dry still matches (cider is still + // selected), so it survives the prune. The style filter still + // narrows the result to just the Dry cider, though — category and + // style filters combine with AND. + controller.toggleCategory('beer'); + expect(controller.selectedStyles, {'Dry'}); + expect(controller.filteredDrinks.map((d) => d.name), ['Crisp Cider']); }); }); @@ -383,7 +430,7 @@ void main() { 'still lists the others', () { controller ..setSource(_sampleDrinks()) - ..setCategory('beer'); + ..toggleCategory('beer'); expect(controller.availableCategories, ['beer', 'cider']); expect(controller.categoryCountsMap, {'beer': 2, 'cider': 2}); }); @@ -398,7 +445,7 @@ void main() { test('availableStyles narrows to the selected category', () { controller ..setSource(_sampleDrinks()) - ..setCategory('cider'); + ..toggleCategory('cider'); expect(controller.availableStyles, ['Dry', 'Sweet']); }); @@ -418,7 +465,7 @@ void main() { test('styleCountsMap narrows to the selected category', () { controller ..setSource(_sampleDrinks()) - ..setCategory('beer'); + ..toggleCategory('beer'); expect(controller.styleCountsMap, {'IPA': 1, 'Bitter': 1}); }); @@ -513,7 +560,7 @@ void main() { allergens: {'nuts': 1}, ), ]) - ..setCategory('beer'); + ..toggleCategory('beer'); expect(controller.availableAllergens, {'gluten'}); }); @@ -590,9 +637,9 @@ void main() { 'count 0', () { controller ..setSource(_sampleDrinks()) - ..setCategory('cider') + ..toggleCategory('cider') ..toggleStyle('IPA'); // IPA has no cider drinks. - expect(controller.selectedCategory, 'cider'); + expect(controller.selectedCategories, {'cider'}); expect(controller.availableCategories, containsAll(['beer', 'cider'])); expect(controller.categoryCountsMap['cider'], 0); expect(controller.categoryCountsMap['beer'], 1); @@ -602,7 +649,7 @@ void main() { 'count 0', () { controller ..setSource(_sampleDrinks()) - ..setCategory('cider') + ..toggleCategory('cider') ..toggleStyle('IPA'); // IPA has no cider drinks. expect(controller.selectedStyles, {'IPA'}); expect( @@ -636,7 +683,7 @@ void main() { ..setShowFavoritesOnly(value: true); expect(controller.categoryCountsMap, {'beer': 1}); - controller.setCategory('beer'); + controller.toggleCategory('beer'); expect(controller.filteredDrinks, hasLength(1)); expect(controller.filteredDrinks.single.name, 'Alpha Ale'); }); @@ -783,14 +830,14 @@ void main() { test('resets category, styles and search but keeps sort/visibility', () { controller ..setSource(_sampleDrinks()) - ..setCategory('beer') + ..toggleCategory('beer') ..toggleStyle('IPA') ..setSearchQuery('alpha') ..setSort(DrinkSort.nameDesc) ..setVisibilityFilter(DrinkVisibilityFilter.notTasted, active: true) ..clearCategoryStyleSearch(); - expect(controller.selectedCategory, isNull); + expect(controller.selectedCategories, isEmpty); expect(controller.selectedStyles, isEmpty); expect(controller.searchQuery, isEmpty); // Preserved diff --git a/test/domain/services/drink_filter_service_test.dart b/test/domain/services/drink_filter_service_test.dart index 88a48dd5..dd277271 100644 --- a/test/domain/services/drink_filter_service_test.dart +++ b/test/domain/services/drink_filter_service_test.dart @@ -87,20 +87,36 @@ void main() { ]; }); - group('filterByCategory', () { - test('filters drinks by category', () { - final result = service.filterByCategory(testDrinks, 'beer').toList(); + group('filterByCategories', () { + test('filters drinks by a single category', () { + final result = service.filterByCategories(testDrinks, { + 'beer', + }).toList(); expect(result, hasLength(3)); expect(result.every((d) => d.category == 'beer'), isTrue); }); - test('returns all drinks when category is null', () { - final result = service.filterByCategory(testDrinks, null).toList(); + test('filters drinks by multiple categories (OR logic)', () { + final result = service.filterByCategories(testDrinks, { + 'beer', + 'cider', + }).toList(); + expect(result, hasLength(5)); + expect( + result.every((d) => d.category == 'beer' || d.category == 'cider'), + isTrue, + ); + }); + + test('returns all drinks when categories set is empty', () { + final result = service.filterByCategories(testDrinks, {}).toList(); expect(result, hasLength(5)); }); - test('returns empty list when no drinks match category', () { - final result = service.filterByCategory(testDrinks, 'mead').toList(); + test('returns empty list when no drinks match any category', () { + final result = service.filterByCategories(testDrinks, { + 'mead', + }).toList(); expect(result, isEmpty); }); }); @@ -469,7 +485,7 @@ void main() { final result = service.filterDrinks( testDrinks, - category: 'beer', + categories: {'beer'}, styles: {'IPA'}, favoritesOnly: true, visibilityFilters: {DrinkVisibilityFilter.availableOnly}, @@ -486,15 +502,23 @@ void main() { }); test('applies only category filter', () { - final result = service.filterDrinks(testDrinks, category: 'cider'); + final result = service.filterDrinks(testDrinks, categories: {'cider'}); expect(result, hasLength(2)); expect(result.every((d) => d.category == 'cider'), isTrue); }); + test('applies multiple categories with OR logic', () { + final result = service.filterDrinks( + testDrinks, + categories: {'beer', 'cider'}, + ); + expect(result, hasLength(5)); + }); + test('applies category and style filters together', () { final result = service.filterDrinks( testDrinks, - category: 'beer', + categories: {'beer'}, styles: {'IPA'}, ); expect(result, hasLength(2)); @@ -511,7 +535,7 @@ void main() { // (only AvailabilityStatus.out is excluded). final result = service.filterDrinks( testDrinks, - category: 'beer', + categories: {'beer'}, visibilityFilters: {DrinkVisibilityFilter.availableOnly}, ); expect(result, hasLength(3)); @@ -520,7 +544,7 @@ void main() { test('returns empty list when filters exclude all drinks', () { final result = service.filterDrinks( testDrinks, - category: 'mead', // No meads in test data + categories: {'mead'}, // No meads in test data ); expect(result, isEmpty); }); diff --git a/test/provider_test.dart b/test/provider_test.dart index 35635fbd..6dce0f31 100644 --- a/test/provider_test.dart +++ b/test/provider_test.dart @@ -540,7 +540,8 @@ void main() { verify(mockAnalyticsService.logFestivalSelected(testFestival)).called(1); }); - test('logs category filter event when category changes', () async { + test('logs category filter event with the canonical joined value when ' + 'multiple categories are selected', () async { final provider = BeerProvider( drinkRepository: mockDrinkRepository, festivalRepository: mockFestivalRepository, @@ -548,11 +549,33 @@ void main() { ); await provider.initialize(); - provider.setCategory('beer'); + provider.toggleCategory('cider'); + verify(mockAnalyticsService.logCategoryFilter('cider')).called(1); - verify(mockAnalyticsService.logCategoryFilter('beer')).called(1); + // Sorted, not selection order, so the logged value is independent of + // which category was toggled first. + provider.toggleCategory('beer'); + verify(mockAnalyticsService.logCategoryFilter('beer,cider')).called(1); }); + test( + 'logs category filter event with null when categories are cleared', + () async { + final provider = BeerProvider( + drinkRepository: mockDrinkRepository, + festivalRepository: mockFestivalRepository, + analyticsService: mockAnalyticsService, + ); + await provider.initialize(); + + provider + ..toggleCategory('beer') + ..clearCategories(); + + verify(mockAnalyticsService.logCategoryFilter(null)).called(1); + }, + ); + test('logs style filter event when style toggles', () async { final provider = BeerProvider( drinkRepository: mockDrinkRepository, diff --git a/test/widgets/drink_filter_sheets_test.dart b/test/widgets/drink_filter_sheets_test.dart index cdba1cef..a988f74e 100644 --- a/test/widgets/drink_filter_sheets_test.dart +++ b/test/widgets/drink_filter_sheets_test.dart @@ -20,6 +20,7 @@ void main() { String id, String name, String style, { + String category = 'beer', Map allergens = const {}, bool? isVegan, }) => Drink( @@ -27,7 +28,7 @@ void main() { id: id, name: name, abv: 5.0, - category: 'beer', + category: category, dispense: 'cask', style: style, allergens: allergens, @@ -74,6 +75,7 @@ void main() { isVegan: true, ), beer('d2', 'Alpha Bitter', 'Bitter'), + beer('d3', 'Crisp Cider', 'Dry', category: 'cider'), ], ); @@ -135,16 +137,45 @@ void main() { testWidgets('CategoryFilterSheet lists categories with counts', ( tester, ) async { - await tester.pumpWidget( - directHost(CategoryFilterSheet(provider: provider)), - ); + await tester.pumpWidget(directHost(const CategoryFilterSheet())); await tester.pumpAndSettle(); expect(find.text('Filter by Category'), findsOneWidget); - expect(find.text('All (2)'), findsOneWidget); - expect(find.textContaining('Beer'), findsOneWidget); + expect(find.text('All (3)'), findsOneWidget); + expect(find.text('Beer (2)'), findsOneWidget); + expect(find.text('Cider (1)'), findsOneWidget); }); + testWidgets( + 'CategoryFilterSheet rows expose semantics label/value/selected', + (tester) async { + provider.toggleCategory('beer'); + await tester.pumpWidget(directHost(const CategoryFilterSheet())); + await tester.pumpAndSettle(); + + expect( + find.byWidgetPredicate( + (widget) => + widget is Semantics && + widget.properties.label == 'Filter by Beer, 2 drinks' && + widget.properties.value == 'Selected' && + widget.properties.selected == true, + ), + findsOneWidget, + ); + expect( + find.byWidgetPredicate( + (widget) => + widget is Semantics && + widget.properties.label == 'Filter by Cider, 1 drinks' && + widget.properties.value == 'Not selected' && + widget.properties.selected == false, + ), + findsOneWidget, + ); + }, + ); + testWidgets('SortOptionsSheet lists every sort option label', ( tester, ) async { @@ -175,7 +206,8 @@ void main() { group('via show* helpers', () { testWidgets( - 'showCategoryFilter: selecting a category sets it and closes', + 'showCategoryFilter: toggling a category selects it without closing ' + 'the sheet', (tester) async { await tester.pumpWidget(launcherHost()); await tester.tap(find.text('open-category')); @@ -184,23 +216,64 @@ void main() { await tester.tap(find.text('Beer (2)')); await tester.pumpAndSettle(); - expect(provider.selectedCategory, 'beer'); - expect(find.text('Filter by Category'), findsNothing); + expect(provider.selectedCategories, {'beer'}); + // Unlike the old single-select radio sheet, the sheet stays open + // so more categories can be toggled. + expect(find.text('Filter by Category'), findsOneWidget); }, ); - testWidgets('showCategoryFilter: selecting All clears the category', ( + testWidgets('showCategoryFilter: toggling two categories reflects both ' + 'selections', (tester) async { + await tester.pumpWidget(launcherHost()); + await tester.tap(find.text('open-category')); + await tester.pumpAndSettle(); + + await tester.tap(find.text('Beer (2)')); + await tester.pumpAndSettle(); + await tester.tap(find.text('Cider (1)')); + await tester.pumpAndSettle(); + + expect(provider.selectedCategories, {'beer', 'cider'}); + expect(find.text('Filter by Category'), findsOneWidget); + + final beerCheckbox = tester.widget( + find.widgetWithText(CheckboxListTile, 'Beer (2)'), + ); + final ciderCheckbox = tester.widget( + find.widgetWithText(CheckboxListTile, 'Cider (1)'), + ); + expect(beerCheckbox.value, isTrue); + expect(ciderCheckbox.value, isTrue); + }); + + testWidgets('showCategoryFilter: selecting All clears the categories', ( tester, ) async { - provider.setCategory('beer'); + provider.toggleCategory('beer'); + await tester.pumpWidget(launcherHost()); + await tester.tap(find.text('open-category')); + await tester.pumpAndSettle(); + + await tester.tap(find.text('All (3)')); + await tester.pumpAndSettle(); + + expect(provider.selectedCategories, isEmpty); + }); + + testWidgets('showCategoryFilter: Clear button removes all selected ' + 'categories', (tester) async { + provider + ..toggleCategory('beer') + ..toggleCategory('cider'); await tester.pumpWidget(launcherHost()); await tester.tap(find.text('open-category')); await tester.pumpAndSettle(); - await tester.tap(find.text('All (2)')); + await tester.tap(find.widgetWithText(TextButton, 'Clear')); await tester.pumpAndSettle(); - expect(provider.selectedCategory, isNull); + expect(provider.selectedCategories, isEmpty); }); testWidgets('showSortOptions: selecting a sort applies it and closes', ( From 95ddc4bc2fdae1eb77d0ff02bfc0bfcb3fcf5058 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 27 Jul 2026 20:46:25 +0000 Subject: [PATCH 2/6] feat(drinks): group styles by category in the style filter sheet With no category selected, the style list was one flat alphabetical run mixing styles from unrelated categories (e.g. wine and perry styles interleaved with beer styles). Add DrinkFilterController.stylesByCategory, a grouped view of the same style facet scope the facet-scoping rule already defines, and render a category header above each group in StyleFilterSheet. A lone group (the common case once a single category is selected) renders flat with no header. Headers reuse the existing "Allergen-free" label treatment and carry Semantics(header: true) for screen-reader section navigation. A selected style outside the current scope is still grouped, under its category in the full source, preserving the facet-scoping invariant that an active filter is never hidden. Fixes #318 --- .../controllers/drink_filter_controller.dart | 50 +++++++++++++ lib/providers/beer_provider.dart | 4 ++ lib/widgets/drink_filter_sheets.dart | 70 +++++++++++++------ .../drink_filter_controller_test.dart | 47 +++++++++++++ test/widgets/drink_filter_sheets_test.dart | 49 +++++++++++++ 5 files changed, 199 insertions(+), 21 deletions(-) diff --git a/lib/domain/controllers/drink_filter_controller.dart b/lib/domain/controllers/drink_filter_controller.dart index 31f64806..5e7f35c6 100644 --- a/lib/domain/controllers/drink_filter_controller.dart +++ b/lib/domain/controllers/drink_filter_controller.dart @@ -1,3 +1,5 @@ +import 'package:collection/collection.dart'; + import '../../models/models.dart'; import '../../utils/string_comparison_helper.dart'; import '../models/models.dart'; @@ -110,6 +112,54 @@ class DrinkFilterController { return styles.toList()..sort(StringComparisonHelper.compareCaseInsensitive); } + /// [availableStyles] grouped by category, for presentation as headed + /// sections in the style filter sheet (issue #318 — a flat, alphabetical + /// style list mixes styles from unrelated categories, e.g. wine and perry + /// styles interleaved with beer styles). Built from the same style facet + /// scope [_scopeFor] already defines — this is not a second scoping rule, + /// just a different shape of the same scoped data. + /// + /// Keys are categories sorted naturally; values are that category's + /// styles, sorted case-insensitively via + /// [StringComparisonHelper.compareCaseInsensitive] — matching + /// [availableStyles]'s own ordering. All ordering lives here, not in the + /// UI. + /// + /// Invariant 1 still applies: a [selectedStyles] entry absent from scope + /// is still included, grouped under the category it carries in the full, + /// unfiltered [_source] (falling back to the first source drink with that + /// style, since scope has none to offer). + /// + /// Counts are deliberately not part of this view — read them from + /// [styleCountsMap], which is scoped identically (per style name, not per + /// category), so the number shown next to a style always matches what + /// ticking it actually yields. A style name occurring under two + /// categories would appear in both groups sharing that one count; this is + /// verified to be zero occurrences in current festival data, but nothing + /// here assumes it can't happen. + Map> get stylesByCategory { + final byCategory = >{}; + for (final drink in _scopeFor(_Facet.style)) { + if (drink.style == null || drink.style!.isEmpty) continue; + byCategory.putIfAbsent(drink.category, () => {}).add(drink.style!); + } + for (final style in _selectedStyles) { + if (byCategory.values.any((styles) => styles.contains(style))) { + continue; + } + final sourceDrink = _source.firstWhereOrNull((d) => d.style == style); + if (sourceDrink != null) { + byCategory.putIfAbsent(sourceDrink.category, () => {}).add(style); + } + } + final sortedCategories = byCategory.keys.toList()..sort(); + return { + for (final category in sortedCategories) + category: byCategory[category]!.toList() + ..sort(StringComparisonHelper.compareCaseInsensitive), + }; + } + /// Drink count per category, scoped per the class doc. Every entry of /// [selectedCategories] with no matches in scope is still present, mapped /// to 0 (invariant 1). diff --git a/lib/providers/beer_provider.dart b/lib/providers/beer_provider.dart index 3853a483..0ab2994c 100644 --- a/lib/providers/beer_provider.dart +++ b/lib/providers/beer_provider.dart @@ -152,6 +152,10 @@ class BeerProvider extends ChangeNotifier { /// Get unique styles from loaded drinks (filtered by category if selected) List get availableStyles => _filter.availableStyles; + /// Get [availableStyles] grouped by category, for the headed style filter + /// sheet sections. See [DrinkFilterController.stylesByCategory]. + Map> get stylesByCategory => _filter.stylesByCategory; + /// Get drink count by category Map get categoryCountsMap => _filter.categoryCountsMap; diff --git a/lib/widgets/drink_filter_sheets.dart b/lib/widgets/drink_filter_sheets.dart index 2e791d99..bd0b10e1 100644 --- a/lib/widgets/drink_filter_sheets.dart +++ b/lib/widgets/drink_filter_sheets.dart @@ -204,8 +204,13 @@ class SortOptionsSheet extends StatelessWidget { } } -/// Style filter sheet with checkboxes for multi-select. Styles arrive already -/// sorted (case-insensitively) from [BeerProvider.availableStyles]. +/// Style filter sheet with checkboxes for multi-select. Styles arrive +/// grouped by category, already sorted, from +/// [BeerProvider.stylesByCategory] — see that getter's doc for the ordering +/// and grouping rules. A category header renders above each group, unless +/// there is exactly one group (the common case once a single category is +/// selected), in which case the list renders flat with no header — a lone +/// header is noise. class StyleFilterSheet extends StatelessWidget { const StyleFilterSheet({super.key}); @@ -215,9 +220,10 @@ class StyleFilterSheet extends StatelessWidget { return Consumer( builder: (context, beerProvider, child) { - final styles = beerProvider.availableStyles; + final stylesByCategory = beerProvider.stylesByCategory; final styleCounts = beerProvider.styleCountsMap; final selectedStyles = beerProvider.selectedStyles; + final showHeaders = stylesByCategory.length > 1; return Container( padding: const EdgeInsets.all(16), @@ -297,24 +303,46 @@ class StyleFilterSheet extends StatelessWidget { child: SingleChildScrollView( child: Column( mainAxisSize: MainAxisSize.min, - children: styles.map((style) { - final count = styleCounts[style] ?? 0; - final isSelected = selectedStyles.contains(style); - return Semantics( - label: 'Filter by $style, $count drinks', - value: isSelected ? 'Selected' : 'Not selected', - selected: isSelected, - button: true, - excludeSemantics: true, - child: CheckboxListTile( - value: isSelected, - onChanged: (_) => beerProvider.toggleStyle(style), - title: Text('$style ($count)'), - controlAffinity: ListTileControlAffinity.leading, - dense: true, - ), - ); - }).toList(), + children: [ + for (final entry in stylesByCategory.entries) ...[ + if (showHeaders) + Semantics( + header: true, + child: Padding( + padding: const EdgeInsets.symmetric( + horizontal: 16, + vertical: 4, + ), + child: Text( + BeverageTypeHelper.formatBeverageType( + entry.key, + ), + style: theme.textTheme.labelMedium?.copyWith( + color: theme.colorScheme.onSurfaceVariant, + ), + ), + ), + ), + ...entry.value.map((style) { + final count = styleCounts[style] ?? 0; + final isSelected = selectedStyles.contains(style); + return Semantics( + label: 'Filter by $style, $count drinks', + value: isSelected ? 'Selected' : 'Not selected', + selected: isSelected, + button: true, + excludeSemantics: true, + child: CheckboxListTile( + value: isSelected, + onChanged: (_) => beerProvider.toggleStyle(style), + title: Text('$style ($count)'), + controlAffinity: ListTileControlAffinity.leading, + dense: true, + ), + ); + }), + ], + ], ), ), ), diff --git a/test/domain/controllers/drink_filter_controller_test.dart b/test/domain/controllers/drink_filter_controller_test.dart index 269e739d..fc89eadf 100644 --- a/test/domain/controllers/drink_filter_controller_test.dart +++ b/test/domain/controllers/drink_filter_controller_test.dart @@ -543,6 +543,53 @@ void main() { }); }); + group('stylesByCategory', () { + test('groups styles under each category, sorted, when no category is ' + 'selected', () { + controller.setSource(_sampleDrinks()); + expect(controller.stylesByCategory, { + 'beer': ['Bitter', 'IPA'], + 'cider': ['Dry', 'Sweet'], + }); + }); + + test('yields a single group when one category is selected', () { + controller + ..setSource(_sampleDrinks()) + ..toggleCategory('beer'); + expect(controller.stylesByCategory, { + 'beer': ['Bitter', 'IPA'], + }); + }); + + test('a selected style absent from scope is still grouped, under its ' + 'category in the full source', () { + controller + ..setSource(_sampleDrinks()) + ..toggleCategory('cider') + ..toggleStyle('IPA'); // IPA is a beer style; cider is selected. + expect(controller.stylesByCategory, { + 'beer': ['IPA'], + 'cider': ['Dry', 'Sweet'], + }); + }); + + test('a style name shared by two categories appears in both groups ' + '(not currently possible with real festival data, but the ' + 'grouping does not assume it can\'t happen)', () { + controller.setSource([ + _drink(id: 'a', name: 'A', category: 'beer', style: 'Dry'), + _drink(id: 'b', name: 'B', category: 'cider', style: 'Dry'), + ]); + expect(controller.stylesByCategory, { + 'beer': ['Dry'], + 'cider': ['Dry'], + }); + // The same style name shares one count, scoped by name not category. + expect(controller.styleCountsMap, {'Dry': 2}); + }); + }); + group('allergen facet scoping', () { test('availableAllergens narrows to the selected category', () { controller diff --git a/test/widgets/drink_filter_sheets_test.dart b/test/widgets/drink_filter_sheets_test.dart index a988f74e..fa6c106b 100644 --- a/test/widgets/drink_filter_sheets_test.dart +++ b/test/widgets/drink_filter_sheets_test.dart @@ -202,6 +202,55 @@ void main() { final ipaY = tester.getTopLeft(find.text('IPA (1)')).dy; expect(bitterY, lessThan(ipaY)); }); + + testWidgets( + 'StyleFilterSheet renders a category header per group when no ' + 'category is selected', + (tester) async { + // No category selected: 2 categories are in scope (beer, cider), + // so headers render. + await tester.pumpWidget(directHost(const StyleFilterSheet())); + await tester.pumpAndSettle(); + + expect(find.text('Beer'), findsOneWidget); + expect(find.text('Cider'), findsOneWidget); + // The Cider header sits above the Dry row (its only style). + final headerY = tester.getTopLeft(find.text('Cider')).dy; + final dryY = tester.getTopLeft(find.text('Dry (1)')).dy; + expect(headerY, lessThan(dryY)); + }, + ); + + testWidgets('StyleFilterSheet renders flat with no header when only one ' + 'category is in scope', (tester) async { + provider.toggleCategory('beer'); + await tester.pumpWidget(directHost(const StyleFilterSheet())); + await tester.pumpAndSettle(); + + expect(find.text('Beer'), findsNothing); + expect(find.text('Bitter (1)'), findsOneWidget); + expect(find.text('IPA (1)'), findsOneWidget); + }); + + testWidgets( + 'StyleFilterSheet category headers are exposed as semantics headers', + (tester) async { + final handle = tester.ensureSemantics(); + try { + await tester.pumpWidget(directHost(const StyleFilterSheet())); + await tester.pumpAndSettle(); + + final headerNode = tester.getSemantics(find.text('Cider')); + expect( + headerNode.flagsCollection.isHeader, + isTrue, + reason: 'category header should carry the isHeader flag', + ); + } finally { + handle.dispose(); + } + }, + ); }); group('via show* helpers', () { From 012c2325f2507befdb3972e6a85f36c8e0abd80e Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 27 Jul 2026 21:15:13 +0000 Subject: [PATCH 3/6] fix(a11y): announce formatted category names in the filter bar The category filter button renders formatted names ('International Beer') but its semanticLabel joined the raw category ids, so a screen reader announced 'international-beer'. Sighted and screen-reader users now get the same names, sorted so the announcement is deterministic (a Set has no defined order). Adds semantic tests pinning the label in all three states. --- lib/screens/drinks_screen.dart | 19 +-- test/drinks_screen_style_filter_test.dart | 145 ++++++++++++++++++++++ 2 files changed, 157 insertions(+), 7 deletions(-) diff --git a/lib/screens/drinks_screen.dart b/lib/screens/drinks_screen.dart index 930ff9ef..88532613 100644 --- a/lib/screens/drinks_screen.dart +++ b/lib/screens/drinks_screen.dart @@ -143,13 +143,18 @@ class _DrinksScreenState extends State { : provider.selectedStyles.length == 1 ? provider.selectedStyles.first : '${provider.selectedStyles.length} styles'; + // Formatted and sorted so the screen reader announces the same names a + // sighted user sees, in a deterministic order (a Set has none). + final formattedCategories = + provider.selectedCategories + .map(BeverageTypeHelper.formatBeverageType) + .toList() + ..sort(); final categoryLabel = provider.selectedCategories.isEmpty ? 'Category' - : provider.selectedCategories.length == 1 - ? BeverageTypeHelper.formatBeverageType( - provider.selectedCategories.first, - ) - : '${provider.selectedCategories.length} categories'; + : formattedCategories.length == 1 + ? formattedCategories.first + : '${formattedCategories.length} categories'; return Container( padding: const EdgeInsets.symmetric(horizontal: 12, vertical: 6), @@ -158,9 +163,9 @@ class _DrinksScreenState extends State { Expanded( child: FilterButton( label: categoryLabel, - semanticLabel: provider.selectedCategories.isEmpty + semanticLabel: formattedCategories.isEmpty ? 'Filter by category' - : 'Filter by category: ${provider.selectedCategories.join(', ')}', + : 'Filter by category: ${formattedCategories.join(', ')}', icon: Icons.filter_list, onPressed: () => showCategoryFilter(context), isActive: provider.selectedCategories.isNotEmpty, diff --git a/test/drinks_screen_style_filter_test.dart b/test/drinks_screen_style_filter_test.dart index 698fbf32..8286ed8e 100644 --- a/test/drinks_screen_style_filter_test.dart +++ b/test/drinks_screen_style_filter_test.dart @@ -5,6 +5,7 @@ import 'package:cambridge_beer_festival/screens/screens.dart'; import 'package:cambridge_beer_festival/models/models.dart'; import 'package:cambridge_beer_festival/providers/providers.dart'; import 'package:cambridge_beer_festival/services/services.dart'; +import 'package:cambridge_beer_festival/widgets/widgets.dart'; import 'package:go_router/go_router.dart'; import 'package:provider/provider.dart'; import 'package:mockito/mockito.dart'; @@ -637,4 +638,148 @@ void main() { provider.dispose(); }); }); + + // The filter bar shows formatted category names ('International Beer'), so + // the screen reader must announce those same names rather than the raw + // category ids used as map keys ('international-beer'). + group('DrinksScreen category filter button label', () { + late MockDrinkRepository mockDrinkRepository; + late MockFestivalRepository mockFestivalRepository; + late MockAnalyticsService mockAnalyticsService; + late BeerProvider provider; + + const testFestival = Festival( + id: 'cbf2025', + name: 'Cambridge Beer Festival 2025', + dataBaseUrl: 'https://test.example.com/cbf2025', + ); + + Drink drinkIn(String category, String id) => Drink( + product: Product( + id: id, + name: 'Drink $id', + abv: 5, + category: category, + dispense: 'cask', + ), + producer: const Producer( + id: 'brewery1', + name: 'Test Brewery', + location: 'Cambridge', + products: [], + ), + festivalId: 'cbf2025', + ); + + setUp(() async { + SharedPreferences.setMockInitialValues({}); + mockDrinkRepository = MockDrinkRepository(); + mockFestivalRepository = MockFestivalRepository(); + mockAnalyticsService = MockAnalyticsService(); + + when(mockFestivalRepository.getFestivals()).thenAnswer( + (_) async => FestivalsResponse( + festivals: [testFestival], + defaultFestivalId: 'cbf2025', + baseUrl: 'https://example.com', + version: '1.0.0', + ), + ); + when( + mockFestivalRepository.getSelectedFestivalId(), + ).thenAnswer((_) async => null); + when(mockDrinkRepository.getDrinks(any)).thenAnswer( + (_) async => [ + drinkIn('international-beer', 'd1'), + drinkIn('cider', 'd2'), + ], + ); + + provider = BeerProvider( + drinkRepository: mockDrinkRepository, + festivalRepository: mockFestivalRepository, + analyticsService: mockAnalyticsService, + ); + await provider.initialize(); + await provider.loadDrinks(); + }); + + tearDown(() { + provider.dispose(); + }); + + // Drink cards render their own category chip, so the category name can + // appear more than once on screen. Scope assertions to the filter bar. + Finder buttonText(String text) => find.descendant( + of: find.byType(FilterButton), + matching: find.text(text), + ); + + Future pumpScreen(WidgetTester tester) async { + await tester.pumpWidget( + ChangeNotifierProvider.value( + value: provider, + child: const MaterialApp(home: DrinksScreen(festivalId: 'cbf2025')), + ), + ); + await tester.pumpAndSettle(); + } + + testWidgets('reads "Category" with no selection', (tester) async { + await pumpScreen(tester); + + expect(buttonText('Category'), findsOneWidget); + expect(find.bySemanticsLabel('Filter by category'), findsOneWidget); + }); + + testWidgets('announces the formatted name for a single selection', ( + tester, + ) async { + await pumpScreen(tester); + provider.toggleCategory('international-beer'); + await tester.pumpAndSettle(); + + expect(buttonText('International Beer'), findsOneWidget); + expect( + find.bySemanticsLabel('Filter by category: International Beer'), + findsOneWidget, + ); + // The raw id must not reach the user, visibly or audibly. + expect(buttonText('international-beer'), findsNothing); + expect( + find.bySemanticsLabel('Filter by category: international-beer'), + findsNothing, + ); + }); + + testWidgets('announces every formatted name, sorted, for a multi ' + 'selection while the visible label counts them', (tester) async { + await pumpScreen(tester); + provider + ..toggleCategory('international-beer') + ..toggleCategory('cider'); + await tester.pumpAndSettle(); + + expect(buttonText('2 categories'), findsOneWidget); + expect( + find.bySemanticsLabel('Filter by category: Cider, International Beer'), + findsOneWidget, + ); + }); + + testWidgets('returns to "Category" once the selection is cleared', ( + tester, + ) async { + await pumpScreen(tester); + provider.toggleCategory('cider'); + await tester.pumpAndSettle(); + expect(buttonText('Cider'), findsOneWidget); + + provider.clearCategories(); + await tester.pumpAndSettle(); + + expect(buttonText('Category'), findsOneWidget); + expect(find.bySemanticsLabel('Filter by category'), findsOneWidget); + }); + }); } From 180f315ce0598872cf0598058f4b0eb5fc0c148e Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 28 Jul 2026 06:51:31 +0000 Subject: [PATCH 4/6] test(drinks): cover the empty-state Clear Filters button The empty-state button is the only way out of a filter combination that matches nothing without reopening the category sheet, and this branch changed its callback from setCategory(null) to clearCategories() with no test exercising it. Asserts the button appears when a category selection yields no drinks, clears the selection when tapped, and disappears afterwards. --- test/drinks_screen_style_filter_test.dart | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/test/drinks_screen_style_filter_test.dart b/test/drinks_screen_style_filter_test.dart index 8286ed8e..9c779ffb 100644 --- a/test/drinks_screen_style_filter_test.dart +++ b/test/drinks_screen_style_filter_test.dart @@ -781,5 +781,27 @@ void main() { expect(buttonText('Category'), findsOneWidget); expect(find.bySemanticsLabel('Filter by category'), findsOneWidget); }); + + // The empty-state button is the only escape hatch from a filter + // combination that matches nothing without reopening the sheet, so it + // has to actually clear the selection. + testWidgets('empty-state Clear Filters button clears the category ' + 'selection', (tester) async { + await pumpScreen(tester); + provider + ..toggleCategory('cider') + ..setSearchQuery('nothing matches this'); + await tester.pumpAndSettle(); + + expect(find.text('No drinks found'), findsOneWidget); + expect(find.text('Clear Filters'), findsOneWidget); + + await tester.tap(find.text('Clear Filters')); + await tester.pumpAndSettle(); + + expect(provider.selectedCategories, isEmpty); + expect(find.text('Clear Filters'), findsNothing); + expect(buttonText('Category'), findsOneWidget); + }); }); } From 3614ec42ee1b5c6e930a7c46de48875a8acf2922 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 28 Jul 2026 06:59:50 +0000 Subject: [PATCH 5/6] fix(a11y): correct filter sheet count grammar and clear-all label MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two screen-reader wording fixes surfaced in review: Category and style filter rows announced '1 drinks'. They now use the count == 1 ? 'drink' : 'drinks' idiom already used in festival_header and my_festival_screen. The empty-state button clears every selected category since this branch made categories multi-select, but announced 'Clear category filter' (singular) and hinted 'show all drinks' — a promise it cannot keep when a search or style filter is also active. --- lib/screens/drinks_screen.dart | 4 ++-- lib/widgets/drink_filter_sheets.dart | 8 +++++-- test/drinks_screen_style_filter_test.dart | 5 ++++ test/widgets/drink_filter_sheets_test.dart | 27 +++++++++++++++++++++- 4 files changed, 39 insertions(+), 5 deletions(-) diff --git a/lib/screens/drinks_screen.dart b/lib/screens/drinks_screen.dart index 88532613..705fef5a 100644 --- a/lib/screens/drinks_screen.dart +++ b/lib/screens/drinks_screen.dart @@ -359,8 +359,8 @@ class _DrinksScreenState extends State { if (provider.selectedCategories.isNotEmpty) ...[ const SizedBox(height: 16), Semantics( - label: 'Clear category filter', - hint: 'Double tap to show all drinks', + label: 'Clear all category filters', + hint: 'Double tap to show every category', button: true, excludeSemantics: true, child: OutlinedButton( diff --git a/lib/widgets/drink_filter_sheets.dart b/lib/widgets/drink_filter_sheets.dart index bd0b10e1..24be80fb 100644 --- a/lib/widgets/drink_filter_sheets.dart +++ b/lib/widgets/drink_filter_sheets.dart @@ -112,7 +112,9 @@ class CategoryFilterSheet extends StatelessWidget { category, ); return Semantics( - label: 'Filter by $formattedCategory, $count drinks', + label: + 'Filter by $formattedCategory, $count ' + '${count == 1 ? 'drink' : 'drinks'}', value: isSelected ? 'Selected' : 'Not selected', selected: isSelected, button: true, @@ -327,7 +329,9 @@ class StyleFilterSheet extends StatelessWidget { final count = styleCounts[style] ?? 0; final isSelected = selectedStyles.contains(style); return Semantics( - label: 'Filter by $style, $count drinks', + label: + 'Filter by $style, $count ' + '${count == 1 ? 'drink' : 'drinks'}', value: isSelected ? 'Selected' : 'Not selected', selected: isSelected, button: true, diff --git a/test/drinks_screen_style_filter_test.dart b/test/drinks_screen_style_filter_test.dart index 9c779ffb..9d83fb33 100644 --- a/test/drinks_screen_style_filter_test.dart +++ b/test/drinks_screen_style_filter_test.dart @@ -795,6 +795,11 @@ void main() { expect(find.text('No drinks found'), findsOneWidget); expect(find.text('Clear Filters'), findsOneWidget); + // The label has to describe clearing every category, not just one. + expect( + find.bySemanticsLabel('Clear all category filters'), + findsOneWidget, + ); await tester.tap(find.text('Clear Filters')); await tester.pumpAndSettle(); diff --git a/test/widgets/drink_filter_sheets_test.dart b/test/widgets/drink_filter_sheets_test.dart index fa6c106b..fbfb8f3a 100644 --- a/test/widgets/drink_filter_sheets_test.dart +++ b/test/widgets/drink_filter_sheets_test.dart @@ -167,7 +167,7 @@ void main() { find.byWidgetPredicate( (widget) => widget is Semantics && - widget.properties.label == 'Filter by Cider, 1 drinks' && + widget.properties.label == 'Filter by Cider, 1 drink' && widget.properties.value == 'Not selected' && widget.properties.selected == false, ), @@ -203,6 +203,31 @@ void main() { expect(bitterY, lessThan(ipaY)); }); + testWidgets('StyleFilterSheet style rows announce a singular drink ' + 'count grammatically', (tester) async { + await tester.pumpWidget(directHost(const StyleFilterSheet())); + await tester.pumpAndSettle(); + + expect( + find.byWidgetPredicate( + (widget) => + widget is Semantics && + widget.properties.label == 'Filter by IPA, 1 drink' && + widget.properties.value == 'Not selected', + ), + findsOneWidget, + ); + // The ungrammatical form must not reach a screen reader. + expect( + find.byWidgetPredicate( + (widget) => + widget is Semantics && + widget.properties.label == 'Filter by IPA, 1 drinks', + ), + findsNothing, + ); + }); + testWidgets( 'StyleFilterSheet renders a category header per group when no ' 'category is selected', From 553a034e953c6c284ac01a374c90cc9d0ed0c841 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 28 Jul 2026 07:31:05 +0000 Subject: [PATCH 6/6] fix(drinks): left-align style filter category headers The headers were the only children narrower than the sheet in a Column that defaults to centre alignment, so they rendered centred instead of sitting above their group. Every other Column in this file already sets CrossAxisAlignment.start. Found by driving a local release web build in a headless browser; the widget tests passed because none of them asserted horizontal position. The header test now pins it. --- lib/widgets/drink_filter_sheets.dart | 4 ++++ test/widgets/drink_filter_sheets_test.dart | 13 +++++++++++++ 2 files changed, 17 insertions(+) diff --git a/lib/widgets/drink_filter_sheets.dart b/lib/widgets/drink_filter_sheets.dart index 24be80fb..a39f23af 100644 --- a/lib/widgets/drink_filter_sheets.dart +++ b/lib/widgets/drink_filter_sheets.dart @@ -305,6 +305,10 @@ class StyleFilterSheet extends StatelessWidget { child: SingleChildScrollView( child: Column( mainAxisSize: MainAxisSize.min, + // The category headers are the only children narrower than + // the sheet; without this they centre instead of sitting + // above their group, unlike every other Column here. + crossAxisAlignment: CrossAxisAlignment.start, children: [ for (final entry in stylesByCategory.entries) ...[ if (showHeaders) diff --git a/test/widgets/drink_filter_sheets_test.dart b/test/widgets/drink_filter_sheets_test.dart index fbfb8f3a..66da9510 100644 --- a/test/widgets/drink_filter_sheets_test.dart +++ b/test/widgets/drink_filter_sheets_test.dart @@ -243,6 +243,19 @@ void main() { final headerY = tester.getTopLeft(find.text('Cider')).dy; final dryY = tester.getTopLeft(find.text('Dry (1)')).dy; expect(headerY, lessThan(dryY)); + + // A header is the only child narrower than the sheet, so it centres + // unless the enclosing Column aligns to the start. Pin it to the + // left: it must start well left of the middle, near its own rows. + final sheetWidth = tester + .getSize(find.byType(StyleFilterSheet)) + .width; + final headerX = tester.getTopLeft(find.text('Cider')).dx; + expect( + headerX, + lessThan(sheetWidth / 4), + reason: 'category header should be left-aligned, not centred', + ); }, );