Add MapSearchErrorPage with location icon for no results found #2320
Conversation
📝 WalkthroughWalkthroughA new file named Changes
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: 1
🧹 Outside diff range and nitpick comments (2)
mobile-v3/lib/src/app/map/pages/map_search_error_page.dart (2)
8-13: Consider adding documentation and improving responsiveness.While the LayoutBuilder approach is good, consider:
- Adding documentation to explain the responsive calculations
- Adding min/max bounds for the relative sizes to ensure readability on extreme screen sizes
+ /// Builds a responsive error page that adapts to different screen sizes. + /// Uses relative sizing with minimum and maximum bounds for better readability. @override Widget build(BuildContext context) { return LayoutBuilder( builder: (context, constraints) { double maxWidth = constraints.maxWidth; double maxHeight = constraints.maxHeight; + // Ensure minimum spacing on very small screens + final minSpacing = maxHeight * 0.02.clamp(10.0, 20.0);
4-43: Consider implementing a more scalable error page pattern.Since this is an error page component, consider creating a reusable base error page widget that can be extended for different error scenarios. This would promote consistency across the app and reduce code duplication for future error pages.
Example approach:
abstract class BaseErrorPage extends StatelessWidget { final String icon; final String primaryMessage; final String secondaryMessage; const BaseErrorPage({ required this.icon, required this.primaryMessage, required this.secondaryMessage, super.key, }); // Common layout logic here } class MapSearchErrorPage extends BaseErrorPage { const MapSearchErrorPage({super.key}) : super( icon: AssetPaths.locationIcon, primaryMessage: AppLocalizations.of(context).noResultsFound, secondaryMessage: AppLocalizations.of(context).tryDifferentLocation, ); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
mobile-v3/assets/icons/location.svgis excluded by!**/*.svg
📒 Files selected for processing (1)
mobile-v3/lib/src/app/map/pages/map_search_error_page.dart(1 hunks)
🔇 Additional comments (3)
mobile-v3/lib/src/app/map/pages/map_search_error_page.dart (3)
1-2: LGTM! Clean and necessary imports.
The imports are minimal and directly related to the widget's functionality.
4-5: Well-structured widget declaration!
Good choice using StatelessWidget as this is a pure presentational component.
17-21: Verify the SVG asset existence.
Ensure the location.svg asset is properly included in the pubspec.yaml assets section.
✅ Verification successful
The previous attempt didn't find the files because we might be in a different directory structure. Let's locate the correct paths first before verifying the asset existence and pubspec.yaml configuration.
The output shows that the location.svg file exists in the correct path (mobile-v3/assets/icons/location.svg). Let's verify its configuration in pubspec.yaml to ensure it's properly declared.
SVG asset verification completed successfully
The location.svg file exists at the correct path and is properly configured in pubspec.yaml under the assets section. The directory 'assets/icons/' is included in the Flutter assets configuration, which makes all SVG files in this directory accessible to the application.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the SVG asset exists and is referenced in pubspec.yaml
if [ -f "assets/icons/location.svg" ]; then
echo "SVG file exists"
else
echo "SVG file missing"
fi
grep -A 5 "flutter:" pubspec.yaml | grep -A 5 "assets:"
Length of output: 181
Script:
#!/bin/bash
# Let's first find the pubspec.yaml file location
fd pubspec.yaml
# Then let's search for SVG files in the repository
fd -e svg
# Let's also check the mobile-v3 directory structure
ls -la mobile-v3
Length of output: 12557
Script:
#!/bin/bash
# Let's check the pubspec.yaml for asset configuration
cat mobile-v3/pubspec.yaml | grep -A 10 "flutter:" | grep -A 10 "assets:"
# Verify the specific SVG file exists
ls -la mobile-v3/assets/icons/location.svg
Length of output: 256
| return Column( | ||
| mainAxisAlignment: MainAxisAlignment.center, | ||
| children: [ | ||
| SvgPicture.asset( | ||
| 'assets/icons/location.svg', | ||
| width: maxWidth * 0.5, | ||
| height: maxWidth * 0.5, | ||
| ), | ||
| SizedBox(height: maxHeight * 0.05), | ||
| Text( | ||
| "No results found", | ||
| style: Theme.of(context).textTheme.headlineLarge!.copyWith( | ||
| fontSize: maxWidth * 0.06, | ||
| fontWeight: FontWeight.bold, | ||
| ), | ||
| ), | ||
| SizedBox(height: maxHeight * 0.02), | ||
| Text( | ||
| "Please try again with a different location name", | ||
| textAlign: TextAlign.center, | ||
| style: TextStyle( | ||
| fontSize: maxWidth * 0.04, | ||
| ), | ||
| ), | ||
| ], | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Several improvements recommended for maintainability and robustness.
While the layout structure is good, consider the following improvements:
- Externalize strings for localization:
+ import 'package:flutter_gen/gen_l10n/app_localizations.dart';
Text(
- "No results found",
+ AppLocalizations.of(context).noResultsFound,
),- Move asset path to constants:
+ // In constants.dart
+ class AssetPaths {
+ static const locationIcon = 'assets/icons/location.svg';
+ }
SvgPicture.asset(
- 'assets/icons/location.svg',
+ AssetPaths.locationIcon,
),- Add error handling for SVG loading:
SvgPicture.asset(
AssetPaths.locationIcon,
width: maxWidth * 0.5,
height: maxWidth * 0.5,
+ placeholderBuilder: (context) => const Icon(Icons.location_on),
),Committable suggestion skipped: line range outside the PR's diff.
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