Refactor home tournament card #141
Conversation
WalkthroughThe changes in this pull request primarily focus on 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 (7)
khelo/lib/ui/flow/home/components/tournament_item.dart (6)
21-28: LGTM! Consider adding documentation.The switch from
backgroundtosizeproperty improves the widget's flexibility for different screen sizes.Add documentation to explain the size property's usage and default behavior:
+ /// Controls the dimensions of the tournament item. + /// If null, defaults to the screen width with a 2:1 aspect ratio. final Size? size;
45-49: Add error handling for palette generation.While the state management is good, consider handling potential errors during palette generation.
imageProvider.createPaletteGenerator().then((palette) { if (mounted) { setState(() => this.palette = palette); } - }); + }).catchError((error) { + if (mounted) { + debugPrint('Failed to generate palette: $error'); + } + });
55-93: Extract constants and improve width calculation.The layout structure looks good, but we can improve maintainability.
+ static const double _aspectRatio = 2.0; + static const double _borderRadius = 16.0; + static const EdgeInsets _contentPadding = EdgeInsets.all(16); + double _calculateWidth(BuildContext context) { + return widget.size?.width ?? context.mediaQuerySize.width; + } @override Widget build(BuildContext context) { - final width = widget.size?.width ?? context.mediaQuerySize.width; + final width = _calculateWidth(context); final (titleColor, dateAndTypeColor) = _getTextColors(); // ... rest of the build method
101-119: Extract gradient constants and reuse height calculation.The gradient implementation is good but could be more maintainable.
+ static const double _gradientOpacityStart = 0.0; + static const double _gradientOpacityMiddle = 0.5; + static const double _gradientOpacityEnd = 1.0; Widget _gradient(BuildContext context, double width) { + final height = width / _aspectRatio; final dominant = palette?.dominantColor?.color ?? context.colorScheme.primary; return Align( alignment: Alignment.bottomCenter, child: Container( - height: width / 2, + height: height, width: width, decoration: BoxDecoration( gradient: LinearGradient( begin: Alignment.topCenter, end: Alignment.bottomCenter, colors: [ - dominant.withOpacity(0), - dominant.withOpacity(0.5), - dominant, + dominant.withOpacity(_gradientOpacityStart), + dominant.withOpacity(_gradientOpacityMiddle), + dominant.withOpacity(_gradientOpacityEnd), ], ), ), ), ); }
124-141: Improve image handling and reuse constants.The image handling is robust but could be more consistent with the rest of the code.
+ static const double _iconSize = 32.0; Widget _bannerImage(String? bannerUrl, double width) { + final height = width / _aspectRatio; return bannerUrl == null ? Center( child: SvgPicture.asset( Assets.images.icTournaments, - height: 32, - width: 32, + height: _iconSize, + width: _iconSize, colorFilter: ColorFilter.mode( context.colorScheme.textPrimary, BlendMode.srcATop, )), ) : CachedNetworkImage( imageUrl: bannerUrl, fit: BoxFit.cover, width: width, - height: width / 2, + height: height, + errorWidget: (context, url, error) => Center( + child: Icon(Icons.error, size: _iconSize), + ), ); }
199-214: Improve color management with named constants and explicit types.The color logic is good but could be more maintainable.
+ static const double _luminanceThreshold = 0.5; + static const double _primaryTextOpacity = 0.8; + static const double _darkTextOpacity = 0.87; + static const double _darkSecondaryTextOpacity = 0.6; - (Color, Color) _getTextColors() { + ({Color primary, Color secondary}) _getTextColors() { final dominant = palette?.dominantColor?.color ?? context.colorScheme.primary; if (dominant == context.colorScheme.primary) { - return ( - context.colorScheme.onPrimary, - context.colorScheme.onPrimary.withOpacity(0.8) - ); + return ( + primary: context.colorScheme.onPrimary, + secondary: context.colorScheme.onPrimary.withOpacity(_primaryTextOpacity) + ); } - if (dominant.computeLuminance() < 0.5) { - return (Colors.white, Colors.white.withOpacity(0.8)); + if (dominant.computeLuminance() < _luminanceThreshold) { + return ( + primary: Colors.white, + secondary: Colors.white.withOpacity(_primaryTextOpacity) + ); } else { - return (Colors.black.withOpacity(0.87), Colors.black.withOpacity(0.6)); + return ( + primary: Colors.black.withOpacity(_darkTextOpacity), + secondary: Colors.black.withOpacity(_darkSecondaryTextOpacity) + ); } }khelo/lib/ui/flow/home/home_screen.dart (1)
164-166: Consider using responsive sizing instead of hardcoded widthThe fixed width of 360 might not be optimal for all screen sizes. Consider making the tournament item size responsive to the screen width.
Here's a suggested implementation:
- size: Size.fromWidth(360), + size: Size.fromWidth(MediaQuery.of(context).size.width * 0.85),Alternatively, you could use
LayoutBuilderfor more fine-grained control:- TournamentItem( - tournament: tournament, - size: Size.fromWidth(360), - margin: EdgeInsets.symmetric(horizontal: 8), - ) + LayoutBuilder( + builder: (context, constraints) { + return TournamentItem( + tournament: tournament, + size: Size.fromWidth(constraints.maxWidth * 0.85), + margin: EdgeInsets.symmetric(horizontal: 8), + ); + }, + )
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
khelo/lib/ui/flow/home/components/tournament_item.dart(3 hunks)khelo/lib/ui/flow/home/home_screen.dart(1 hunks)khelo/lib/ui/flow/home/view_all/home_view_all_screen.dart(1 hunks)
🔇 Additional comments (1)
khelo/lib/ui/flow/home/view_all/home_view_all_screen.dart (1)
84-84: 🛠️ Refactor suggestion
Add size parameter to TournamentItem for consistent behavior
The TournamentItem widget requires a size parameter for proper layout management, as seen in other screens. Consider adding it here to maintain consistency.
Apply this diff:
-return TournamentItem(tournament: tournament);
+return TournamentItem(
+ tournament: tournament,
+ size: Size.fromWidth(360),
+);Let's verify the usage of TournamentItem across the codebase:
# Conflicts: # khelo/lib/ui/flow/home/components/tournament_item.dart
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
khelo/lib/ui/flow/home/components/tournament_item.dart (2)
115-133: Consider extracting gradient colors to theme.The gradient implementation looks good, but consider moving the opacity values and gradient stops to the theme for consistency across the app.
+ // In your theme file + class AppGradients { + static const tournamentItemOverlay = [0.0, 0.5, 1.0]; + } // In this file decoration: BoxDecoration( gradient: LinearGradient( begin: Alignment.topCenter, end: Alignment.bottomCenter, colors: [ - dominant.withOpacity(0), - dominant.withOpacity(0.5), - dominant, + dominant.withOpacity(AppGradients.tournamentItemOverlay[0]), + dominant.withOpacity(AppGradients.tournamentItemOverlay[1]), + dominant.withOpacity(AppGradients.tournamentItemOverlay[2]), ], ), ),
138-155: Consider extracting the aspect ratio constant.The aspect ratio calculation (width/2) is duplicated. Consider extracting it to a constant.
+ // At the top of the class + static const _aspectRatio = 2.0; Widget _bannerImage(String? bannerUrl, double width) { return bannerUrl == null ? Center( child: SvgPicture.asset( Assets.images.icTournaments, height: 32, width: 32, colorFilter: ColorFilter.mode( context.colorScheme.textPrimary, BlendMode.srcATop, ), ), ) : CachedNetworkImage( imageUrl: bannerUrl, fit: BoxFit.cover, width: width, - height: width / 2, + height: width / _aspectRatio, );
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
khelo/lib/ui/flow/home/components/tournament_item.dart(5 hunks)
🔇 Additional comments (4)
khelo/lib/ui/flow/home/components/tournament_item.dart (4)
48-52: LGTM! Improved palette generation logic.
The palette generation code has been simplified while maintaining proper lifecycle management with the mounted check.
69-107: LGTM! Improved layout structure with better visual hierarchy.
The Stack-based layout with proper alignment and padding provides a cleaner implementation.
213-229: LGTM! Good contrast handling in text colors.
The text color selection based on luminance ensures good readability across different background colors.
21-21: Verify the impact of API changes across the codebase.
The replacement of background with size property is a breaking change that could affect existing usages of this widget.
Also applies to: 25-25
✅ Verification successful
Based on the search results, I can now generate the final response:
API change is properly handled across the codebase
The removal of background property and addition of optional size property is safe as all existing usages have been updated:
- In
home_screen.dart: Uses the newsizeproperty correctly withSize.fromWidth(360) - In
home_view_all_screen.dart: Uses the default behavior without size, which is valid since it's optional
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for existing TournamentItem instantiations that might need updates
rg -l "TournamentItem\(" | xargs rg -A 5 "TournamentItem\("
Length of output: 1629
tournament.card.webm
Summary by CodeRabbit
New Features
sizeproperty.Bug Fixes
Refactor