Fix: Properly Handle Internet Connectivity in CustomProgressDialog Widget #2736
Conversation
WalkthroughThe changes update the Changes
Assessment against linked issues
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (1)
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
Documentation and Community
|
Our Pull Request Approval ProcessThanks for contributing! Testing Your CodeRemember, your PRs won't be reviewed until these criteria are met:
Our policies make our code better. ReviewersDo not assign reviewers. Our Queue Monitors will review your PR and assign them.
Reviewing Your CodeYour reviewer(s) will have the following roles:
Other🎯 Please be considerate of our volunteers' time. Contacting the person who assigned the reviewers is not advised unless they ask for your input. Do not @ the person who did the assignment otherwise. |
![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 (1)
lib/widgets/custom_progress_dialog.dart (1)
28-56: LGTM! Proper implementation of connectivity-based UI states.The implementation correctly:
- Uses model.connectivityPresent for conditional rendering
- Shows CupertinoActivityIndicator when online
- Shows no-internet image and localized text when offline
A few suggestions to enhance the implementation:
- Consider extracting the no-internet UI into a separate widget for better maintainability
- Consider adding a fade transition between states for better UX
+ // Extract to a separate widget + class NoInternetContent extends StatelessWidget { + @override + Widget build(BuildContext context) { + return Column( + children: [ + Container( + alignment: Alignment.bottomCenter, + height: SizeConfig.screenWidth! * 0.4, + width: SizeConfig.screenWidth! * 0.4, + padding: const EdgeInsets.only(bottom: 10), + decoration: const BoxDecoration( + image: DecorationImage( + image: AssetImage('assets/images/no_internet.png'), + fit: BoxFit.scaleDown, + ), + ), + ), + Text( + '${AppLocalizations.of(context)!.strictTranslate("No Internet")}!', + style: Theme.of(context).textTheme.headlineSmall, + ), + ], + ); + } + } // In CustomProgressDialog child: AnimatedSwitcher( duration: const Duration(milliseconds: 300), child: model.connectivityPresent ? Center( child: CupertinoActivityIndicator( radius: SizeConfig.screenWidth! * 0.065, ), ) : const NoInternetContent(), ),
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
lib/widgets/custom_progress_dialog.dart(2 hunks)test/widget_tests/widgets/custom_progress_dialog_test.dart(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Checking codebase
🔇 Additional comments (6)
lib/widgets/custom_progress_dialog.dart (1)
8-10: LGTM! Documentation accurately reflects the widget's behavior.The documentation has been updated to correctly describe the widget's behavior for both online and offline states.
test/widget_tests/widgets/custom_progress_dialog_test.dart (5)
50-74: LGTM! Comprehensive styling and layout tests.The test properly verifies:
- Container border radius
- Container margins based on screen dimensions
75-84: LGTM! Online state verification.The test correctly verifies the presence of CupertinoActivityIndicator when online.
86-98: LGTM! Activity indicator size verification.The test properly verifies the size of the activity indicator based on screen width.
100-121: LGTM! Offline state verification.The test correctly verifies:
- Absence of activity indicator
- Presence of Column layout
- Display of "No Internet!" message
123-150: LGTM! No-internet image properties verification.The test thoroughly verifies:
- Container alignment
- Image dimensions
- Correct asset path
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop-postgres #2736 +/- ##
=================================================
Coverage 96.37% 96.38%
=================================================
Files 189 189
Lines 10020 10029 +9
=================================================
+ Hits 9657 9666 +9
Misses 363 363 ☔ View full report in Codecov by Sentry. |
|
@palisadoes There is some issue in docusaurus deployment. EDIT : |
|
Yes please fix the broken links |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
docs/docs/auto-docs/widgets_custom_list_tile/CustomListTile/index.md (1)
12-14: Property Information and Usage ExplanationThe documentation indicates that the index property is of type int and is marked as "read / write". Consider expanding this section with a brief description of the property's purpose or usage context within the widget. This could provide additional clarity for developers who are new to the widget.
docs/docs/auto-docs/widgets_post_widget/PostContainerState/build.md (2)
12-17: Documentation Annotation Consistency
The file uses two different representations for the override annotation (a linked '@[override]' on line 12 and an HTML span on line 17). Consider unifying these to a single, consistent style for clarity in the documentation.
60-74: Refactor Suggestion: Use Stable Key and Simplify Boolean Assignment
Within the build method, a new key is generated on every build withKey(Random().nextInt(1000).toString()). This approach may lead to unnecessary widget rebuilds. Instead, consider using a stable key that remains consistent across builds. Additionally, the ternary assignment on line 65 can be simplified to:
inView = info.visibleFraction > 0.5;
This improves readability and conciseness.docs/docs/auto-docs/widgets_pinned_carousel_widget/CustomCarouselScrollerState/build.md (2)
12-17: Documentation Annotation Consistency
Similar to the previous file, the override annotation is presented as a linked text on line 12 and as an HTML span on line 17. Unify these to a consistent style throughout the documentation for better clarity.
60-132: UI Enhancement Suggestion: Dynamic Indicators and Text Overflow Handling
In the build method implementation, consider the following improvements:
• Instead of manually truncating the description text by taking a substring (lines 83–85), leverage the Text widget’s built-in overflow property (e.g., TextOverflow.ellipsis) to handle varying text lengths more gracefully.
• The divider indicators are generated with a hard-coded count of 4. If the indicators are meant to reflect the number of pinned posts or should otherwise be configurable, dynamically generating the count fromwidget.pinnedPosts.lengthmight offer better flexibility.
These adjustments can improve responsiveness and maintainability of the UI.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
docs/docs/auto-docs/widgets_custom_list_tile/CustomListTile/index.md(1 hunks)docs/docs/auto-docs/widgets_pinned_carousel_widget/CustomCarouselScrollerState/build.md(1 hunks)docs/docs/auto-docs/widgets_post_widget/PostContainerState/build.md(1 hunks)
🔇 Additional comments (1)
docs/docs/auto-docs/widgets_custom_list_tile/CustomListTile/index.md (1)
4-4: Clear and Concise TitleThe header "# index property" clearly identifies the document section. This helps users quickly locate the documentation for the index property.
|
@palisadoes @noman2002 Please review. |
palisadoes
merged commit 392b929
into
PalisadoesFoundation:develop-postgres
What kind of change does this PR introduce?
Issue Number:
Fixes #2735
Did you add tests for your changes?
Yes
Tests are written for all changes made in this PR.
Test coverage meets or exceeds the current coverage (~90/95%).
Snapshots/Videos:
Summary
Does this PR introduce a breaking change?
Checklist for Repository Standards
coderaabbitaireview suggestions?Have you read the contributing guide?
Summary by CodeRabbit
New Features
indexproperty in theCustomListTilewidget for enhanced functionality.buildmethod for theCustomCarouselScrollerStateandPostContainerStateclasses to improve the rendering of carousel and post widgets.Tests