Enhance responsiveness of AnalyticsForecastWidget by adding dynamic h… #2391
Conversation
…eight, icon size, and margin calculations based on screen dimensions.
📝 WalkthroughWalkthroughThe pull request focuses on enhancing the responsiveness of the Changes
Possibly related PRs
Suggested Labels
Suggested Reviewers
Poem
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
🧹 Nitpick comments (5)
mobile-v3/lib/src/app/dashboard/widgets/analytics_forecast_widget.dart (5)
28-33: Consider adding documentation and handling orientation changes.The height calculation looks good, but could benefit from:
- Documentation explaining why 10% and the clamp values (60.0, 100.0) were chosen
- Handling orientation changes explicitly
+ /// Calculates the responsive height based on screen dimensions. + /// Uses 10% of screen height, clamped between 60-100 logical pixels + /// to ensure readability across different device sizes. double _getResponsiveHeight(BuildContext context) { final screenHeight = MediaQuery.of(context).size.height; + final orientation = MediaQuery.of(context).orientation; + final baseHeight = orientation == Orientation.portrait + ? screenHeight * 0.1 + : screenHeight * 0.15; - final height = screenHeight * 0.1; // 10% of screen height - return height.clamp(60.0, 100.0); // Min 60, max 100 + return baseHeight.clamp(60.0, 100.0); }
35-46: Extract magic numbers into named constants.The responsive calculations use several magic numbers. Consider extracting these into named constants at the class level for better maintainability.
+ // At the top of the class + static const double _MIN_ICON_SIZE = 20.0; + static const double _MAX_ICON_SIZE = 30.0; + static const double _ICON_WIDTH_FACTOR = 0.04; + + static const double _MIN_MARGIN = 2.0; + static const double _MAX_MARGIN = 8.0; + static const double _MARGIN_WIDTH_FACTOR = 0.01; double _getResponsiveIconSize(BuildContext context) { final screenWidth = MediaQuery.of(context).size.width; - final iconSize = screenWidth * 0.04; - return iconSize.clamp(20.0, 30.0); + final iconSize = screenWidth * _ICON_WIDTH_FACTOR; + return iconSize.clamp(_MIN_ICON_SIZE, _MAX_ICON_SIZE); } double _getResponsiveMargin(BuildContext context) { final screenWidth = MediaQuery.of(context).size.width; - return (screenWidth * 0.01).clamp(2.0, 8.0); + return (screenWidth * _MARGIN_WIDTH_FACTOR).clamp(_MIN_MARGIN, _MAX_MARGIN); }
72-76: Extract ShimmerContainer width calculation logic.The width calculation uses a magic number (8). Consider extracting this into a constant or a method for consistency with other responsive calculations.
+ static const double _SHIMMER_WIDTH_DIVISOR = 8.0; + + double _getResponsiveShimmerWidth(double maxWidth) { + return maxWidth / _SHIMMER_WIDTH_DIVISOR; + } return ShimmerContainer( height: _getResponsiveHeight(context), borderRadius: 22, - width: constraints.maxWidth / 8, + width: _getResponsiveShimmerWidth(constraints.maxWidth), );
81-91: Use consistent responsive approach for error state.The error state's text size uses a different responsive approach (3% of screen width) compared to other calculations. Consider using a similar pattern to _getResponsiveHeight for consistency.
+ double _getResponsiveTextSize(BuildContext context) { + final screenWidth = MediaQuery.of(context).size.width; + return (screenWidth * 0.03).clamp(12.0, 18.0); + } return Container( height: _getResponsiveHeight(context), child: Center( child: Text( state.toString(), style: Theme.of(context).textTheme.bodyMedium?.copyWith( - fontSize: MediaQuery.of(context).size.width * 0.03, + fontSize: _getResponsiveTextSize(context), ), ), ), );
119-121: Consider caching fontSize calculation.The fontSize calculation could be moved to a final variable at class level since it depends only on the height parameter.
class ForeCastChip extends StatelessWidget { + late final double _fontSize; + + ForeCastChip({...}) { + _fontSize = (height * 0.2).clamp(12.0, 16.0); + } @override Widget build(BuildContext context) { final textScaleFactor = MediaQuery.of(context).textScaleFactor; - final fontSize = (height * 0.2).clamp(12.0, 16.0); // ... in Text widget ... style: TextStyle( - fontSize: fontSize * textScaleFactor, + fontSize: _fontSize * textScaleFactor, fontWeight: active ? FontWeight.bold : FontWeight.normal, ),Also applies to: 138-144
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
mobile-v3/lib/src/app/dashboard/widgets/analytics_forecast_widget.dart(3 hunks)
🔇 Additional comments (1)
mobile-v3/lib/src/app/dashboard/widgets/analytics_forecast_widget.dart (1)
28-46: Add unit tests for responsive calculations.The responsive calculation methods would benefit from unit tests to verify their behavior across different screen sizes and orientations.
Consider adding tests for:
- Portrait vs landscape calculations
- Various screen sizes
- Edge cases (very small/large screens)
- Clamp value boundaries
Would you like me to help generate the test cases?
…eight, icon size, and margin calculations based on screen dimensions.
Summary of Changes (What does this PR do?)
Status of maturity (all need to be checked before merging):
How should this be manually tested?
What are the relevant tickets?
Screenshots (optional)
Summary by CodeRabbit
New Features
Improvements