Tournament minor improvements #131
Conversation
WalkthroughThe changes in this pull request involve updates to the localization file and several Dart files related to the tournament detail feature. A new key for statistics has been added to the localization file, while the Changes
Possibly related PRs
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (5)
style/lib/button/more_option_button.dart (1)
15-26: Consider extracting hardcoded dimensions to named constants.While the implementation is good, the hardcoded dimensions (28x28) could be extracted to named constants at the top of the file for better maintainability and reusability.
import 'package:flutter/material.dart'; import 'package:style/button/action_button.dart'; import 'package:style/extensions/context_extensions.dart'; + +const double _kMoreOptionButtonSize = 28.0; Widget moreOptionButton( BuildContext context, { Function()? onPressed, double size = 24, Color? tintColor, Color? backgroundColor, }) { return actionButton(context, onPressed: onPressed, icon: Container( - height: 28, - width: 28, + height: _kMoreOptionButtonSize, + width: _kMoreOptionButtonSize, decoration: BoxDecoration(khelo/lib/ui/flow/tournament/detail/tournament_detail_view_model.dart (1)
Line range hint
119-179: Consider optimizing the filter logicWhile the implementation is functionally correct, the filter logic could be optimized to avoid unnecessary iterations when
KeyStatFilterTag.allis selected.Consider this optimization:
void onStatFilter(KeyStatFilterTag tag) { if (state.tournament == null) return; var filteredStats = state.tournament!.keyStats; + + if (tag == KeyStatFilterTag.all) { + state = state.copyWith( + filteredStats: filteredStats, + selectedFilterTag: tag, + ); + return; + } filteredStats = filteredStats.where((e) { switch (tag) { - case KeyStatFilterTag.all: - return true; case KeyStatFilterTag.runs: return (e.stats.battingStat?.runScored ?? 0) > 0; // ... rest of the caseskhelo/lib/ui/flow/tournament/detail/tournament_detail_screen.dart (2)
255-267: LGTM: Well-implemented gradient overlay.The gradient overlay enhances text readability and provides a smooth visual transition. Consider extracting the gradient colors to a theme constant for consistency across the app.
// Consider creating a theme constant: static const kHeaderGradientColors = [ Colors.transparent, Colors.black26, Colors.black87, ];
379-395: Consider making the back button more reusable.The back button implementation is good but could be made more reusable for other screens. Consider extracting it to a shared component.
// Consider moving to a shared component: class AppBackButton extends StatelessWidget { final VoidCallback? onPressed; final Color? backgroundColor; const AppBackButton({ Key? key, this.onPressed, this.backgroundColor, }) : super(key: key); @override Widget build(BuildContext context) { return actionButton( context, onPressed: onPressed ?? context.pop, icon: Container( height: 28, width: 28, decoration: BoxDecoration( shape: BoxShape.circle, color: backgroundColor ?? context.colorScheme.containerHighOnSurface.withOpacity(0.3), ), child: Icon( Platform.isIOS ? Icons.arrow_back_ios : Icons.arrow_back, size: 20, color: context.colorScheme.textPrimary, ), ), ); } }khelo/lib/ui/flow/tournament/detail/tabs/tournament_detail_overview_tab.dart (1)
54-54: Consider using animateToPage for smoother navigationCurrently,
jumpToPageis used for navigation, which can be jarring for users. Consider usinganimateToPagefor smoother transitions.Example implementation:
- onViewAll: () => controller.jumpToPage(2), + onViewAll: () => controller.animateToPage( + 2, + duration: Duration(milliseconds: 300), + curve: Curves.easeInOut, + ),Also applies to: 168-168, 267-267
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
khelo/assets/locales/app_en.arb(1 hunks)khelo/lib/ui/flow/tournament/detail/tabs/tournament_detail_overview_tab.dart(5 hunks)khelo/lib/ui/flow/tournament/detail/tournament_detail_screen.dart(9 hunks)khelo/lib/ui/flow/tournament/detail/tournament_detail_view_model.dart(5 hunks)khelo/lib/ui/flow/tournament/detail/tournament_detail_view_model.freezed.dart(1 hunks)style/lib/button/more_option_button.dart(1 hunks)
🔇 Additional comments (12)
style/lib/button/more_option_button.dart (1)
11-11: LGTM! Clean parameter addition.
The new optional backgroundColor parameter enhances the button's customizability while maintaining backward compatibility.
khelo/lib/ui/flow/tournament/detail/tournament_detail_view_model.dart (3)
56-56: LGTM: Improved state consistency
The change ensures consistent state management by using the current filter tag instead of null.
227-227: LGTM: Appropriate default filter
Setting KeyStatFilterTag.all as the default filter aligns with the PR objective of showing all statistics by default.
234-234: Verify localization key availability
The new filter tag uses the localization key key_stat_all. Let's verify its existence across all localization files.
Also applies to: 247-248
✅ Verification successful
Let me check the content of the English ARB file and verify if there are other locale files that might be missing the key.
Localization key key_stat_all needs to be added to other locale files
The localization key key_stat_all is only present in the English locale file (app_en.arb), but there appears to be no other locale files in the project. However, if the app is intended to support multiple languages, corresponding translations should be added when new locale files are created.
- ✓
key_stat_allis properly defined inkhelo/assets/locales/app_en.arb - ℹ️ No other locale files exist currently
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the key_stat_all localization key exists in all .arb files
# Find all .arb files and check for the key_stat_all key
fd -e arb -x sh -c 'echo "Checking {}" && grep -l "key_stat_all" {} || echo "Missing in {}"'
Length of output: 171
Script:
#!/bin/bash
# First, let's see all available locale files
fd -e arb
# Then, let's check the content of the English ARB file
cat ./khelo/assets/locales/app_en.arb | grep -A 1 "key_stat_all"
Length of output: 167
khelo/lib/ui/flow/tournament/detail/tournament_detail_screen.dart (2)
1-2: LGTM: Import changes are appropriate.
The addition of the action button import aligns with the new back button implementation.
Also applies to: 22-22
117-117: LGTM: Enhanced app bar styling and navigation.
The changes improve the UI by:
- Adding a back button for better navigation
- Styling the more options button with appropriate size and background opacity for better visibility
Also applies to: 125-128
khelo/lib/ui/flow/tournament/detail/tabs/tournament_detail_overview_tab.dart (4)
18-26: Good refactoring to ConsumerWidget!
The change from ConsumerStatefulWidget to ConsumerWidget simplifies the implementation since no local state is needed. The addition of PageController improves navigation between tabs.
66-109: Nice UI/UX improvement with OnTapScale!
The addition of OnTapScale provides better tactile feedback, and the navigation to match details is well implemented. The UI structure is clean and follows material design principles.
114-115: LGTM: Proper method signature update!
The addition of BuildContext as the first parameter follows Flutter conventions correctly.
Line range hint 149-150: Verify null safety in key stats sorting
The current sorting implementation might throw when comparing null values. Consider adding null safety:
- ..sort((a, b) => b.value?.compareTo(a.value ?? 0) ?? 0);
+ ..sort((a, b) => (b.value ?? 0).compareTo(a.value ?? 0));Let's verify if this is a potential issue by checking other usages:
khelo/lib/ui/flow/tournament/detail/tournament_detail_view_model.freezed.dart (1)
252-252: Reminder: This is a generated file.
While the change to use KeyStatFilterTag.all as the default filter aligns with the PR objectives, this file is auto-generated and shouldn't be modified directly. The change should be made in the source file tournament_detail_view_model.dart instead.
Let's verify the related changes:
✅ Verification successful
The changes in the generated file are correct and consistent with the source
The verification confirms that:
- The
KeyStatFilterTagenum in the source file includes thealloption - The default value in the source file is set to
KeyStatFilterTag.all - The localization file includes the
key_stat_alltranslation
The modification in the generated .freezed.dart file is a result of proper changes in the source files, so there's no issue with the generated code.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the source changes for KeyStatFilterTag enum and default value
# Test 1: Check the source file for KeyStatFilterTag enum and default value
echo "Checking source file for KeyStatFilterTag enum and default value..."
rg -A 5 "KeyStatFilterTag" "lib/ui/flow/tournament/detail/tournament_detail_view_model.dart"
# Test 2: Check localization file for the new "all" key
echo "Checking localization file for new key..."
rg "key_stat_all" "lib/l10n/app_en.arb"
Length of output: 530
Script:
#!/bin/bash
# Let's first find the correct paths using fd
echo "Finding source files..."
fd -t f "tournament_detail_view_model" --exclude "*.freezed.dart" --exclude "*.g.dart"
echo -e "\nFinding localization files..."
fd "app_en.arb"
echo -e "\nLet's also check for enum definition..."
rg -p "enum KeyStatFilterTag"
Length of output: 564
Script:
#!/bin/bash
# Now that we have the correct paths, let's check the content
echo "Checking KeyStatFilterTag enum definition and usage..."
rg -A 10 "enum KeyStatFilterTag" "khelo/lib/ui/flow/tournament/detail/tournament_detail_view_model.dart"
echo -e "\nChecking the source file for default value..."
rg "selectedFilterTag" "khelo/lib/ui/flow/tournament/detail/tournament_detail_view_model.dart"
echo -e "\nChecking localization file for 'all' key..."
rg "all" "khelo/assets/locales/app_en.arb"
Length of output: 3566
khelo/assets/locales/app_en.arb (1)
243-243: LGTM! The new localization key is properly added.
The addition of "key_stat_all": "All" is well-placed within the related stat filter keys and follows the established naming conventions.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
khelo/lib/ui/flow/tournament/detail/tournament_detail_screen.dart (1)
256-268: Consider making the gradient colors theme-aware.While the gradient overlay effectively improves text readability, consider deriving the colors from the theme instead of using hardcoded
surfaceDarkColorto ensure consistent appearance across different theme modes.decoration: BoxDecoration( gradient: LinearGradient( begin: Alignment.topCenter, end: Alignment.bottomCenter, colors: [ Colors.transparent, - surfaceDarkColor.withOpacity(0.2), - surfaceDarkColor.withOpacity(0.8), + context.colorScheme.surface.withOpacity(0.2), + context.colorScheme.surface.withOpacity(0.8), ], ), ),khelo/lib/ui/flow/tournament/detail/tournament_detail_view_model.dart (1)
Line range hint
119-135: Ensure null safety when accessing stats propertiesWhile filtering
filteredStats, you are accessing nullable properties likee.stats.battingStat?.runScored. Although you are using null-aware operators, it's crucial to ensure that all nullable accesses are safely handled to prevent potential null reference errors.Consider reviewing all nullable property accesses and ensure default values are appropriately used.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
khelo/lib/ui/flow/tournament/detail/tournament_detail_screen.dart(10 hunks)khelo/lib/ui/flow/tournament/detail/tournament_detail_view_model.dart(5 hunks)
🔇 Additional comments (7)
khelo/lib/ui/flow/tournament/detail/tournament_detail_screen.dart (2)
380-396: LGTM: Well-implemented platform-aware back button.
The back button implementation follows platform design guidelines with:
- Platform-specific icons (iOS vs Android)
- Proper styling with semi-transparent background
- Correct navigation handling
126-129: LGTM: Consistent styling with the back button.
The more options button styling maintains visual consistency with the back button through:
- Matching background color opacity (0.4)
- Consistent size parameters
- Proper use of theme color scheme
khelo/lib/ui/flow/tournament/detail/tournament_detail_view_model.dart (5)
56-56: Good call to onStatFilter after loading the tournament
Calling onStatFilter(state.selectedFilterTag); in the loadTournament() method ensures that the statistics are filtered according to the selected filter tag as soon as the tournament data is loaded.
126-127: Properly handle the 'All' filter option in stats filtering
Adding the KeyStatFilterTag.all case with return true; ensures that all stats are included when the 'All' option is selected, providing a comprehensive view to the user.
231-231: Set default filter tag to 'All' for broader initial display
By setting @Default(KeyStatFilterTag.all) for selectedFilterTag, users will see all statistics by default upon accessing the tournament details, enhancing the user experience.
238-238: Add 'All' option to the KeyStatFilterTag enum
Including all in the KeyStatFilterTag enum allows users to select an option that displays all key statistics without any filters applied.
251-252: Localize the 'All' filter option in getString method
Adding the KeyStatFilterTag.all case in the getString method and returning context.l10n.key_stat_all ensures the 'All' option is correctly localized, maintaining consistency in the app's language support.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
khelo/lib/ui/flow/tournament/detail/tournament_detail_view_model.dart(5 hunks)
🔇 Additional comments (2)
khelo/lib/ui/flow/tournament/detail/tournament_detail_view_model.dart (2)
56-56: LGTM: Consistent filter application on reload
Good addition to maintain filter consistency when tournament data is reloaded.
227-227: LGTM: Clean implementation of "all" filter option
The addition of the "all" filter tag and its integration into the state management is well-implemented and aligns with the PR objectives.
Also applies to: 234-234, 247-248
minor.improvments.webm
Summary by CodeRabbit
Release Notes
New Features
moreOptionButtonwith a customizable background color.Improvements
Bug Fixes