Merged
Conversation
Contributor
Reviewer's Guide by SourceryThis pull request comprehensively reworks the app’s responsive layout and navigation behavior. The implementation shifts from a fixed notion of 'single/dual' layouts based on LayoutState and ScreenLayout to a more flexible model using ViewSize and LayoutMode. Changes propagate throughout routing, settings screens, and various UI components so that all views become responsive on every device irrespective of the original discrete layouts. Class diagram for HomeSettingsModel updatesclassDiagram
class HomeSettingsModel {
<<immutable>>
Set~LayoutMode~ screenLayouts
Set~ViewSize~ layoutStates
HomeBanner homeBanner
HomeCarouselSettings carouselSettings
HomeNextUp nextUp
+fromJson(json: Map)
+toJson(): Map
+copyWith(...): HomeSettingsModel
}
class LayoutMode {
<<enumeration>>
single
dual
+label(context): String
}
class ViewSize {
<<enumeration>>
phone
tablet
desktop
+label(context): String
}
HomeSettingsModel --> LayoutMode : uses
HomeSettingsModel --> ViewSize : uses
note for HomeSettingsModel "Now includes new fields screenLayouts and layoutStates\nwith default values set to all possible enum values"
class Utils {
+selectAvailableOrSmaller<T>(value, availableOptions, allOptions): T
}
HomeSettingsModel ..> Utils : uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey @PartyDonut - I've reviewed your changes - here's some feedback:
Overall Comments:
- Consider caching extracted values from AdaptiveLayout (like viewSize and layoutMode) early in build to reduce repeated method calls.
- Ensure consistent usage of the new AdaptiveLayout methods (viewSizeOf and layoutModeOf) instead of mixing with the old AdaptiveLayout.of patterns.
- Extract common dialog and onTap patterns in client settings sections to simplify and reduce repetitive boilerplate code.
Here's what I looked at during the review
- 🟡 General issues: 3 issues found
- 🟢 Security: all looks good
- 🟢 Review instructions: all looks good
- 🟢 Testing: all looks good
- 🟢 Complexity: all looks good
- 🟢 Documentation: all looks good
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| import 'package:fladder/widgets/shared/fladder_slider.dart'; | ||
|
|
||
| List<Widget> buildClientSettingsVisual(BuildContext context, WidgetRef ref) { | ||
| late final nextUpDaysEditor = TextEditingController( |
Contributor
There was a problem hiding this comment.
suggestion: Consider moving TextEditingController initialization out of the build method.
Instantiating controllers within build() causes them to be recreated on every rebuild, which can lead to loss of user input and unnecessary object creation. Consider moving them to a stateful widget’s initState and disposing them appropriately.
Suggested implementation:
import 'package:flutter/material.dart';
import 'package:flutter_riverpod/flutter_riverpod.dart';
class ClientSettingsVisual extends ConsumerStatefulWidget {
const ClientSettingsVisual({Key? key}) : super(key: key);
@override
ClientSettingsVisualState createState() => ClientSettingsVisualState();
}
class ClientSettingsVisualState extends ConsumerState<ClientSettingsVisual> {
late final TextEditingController nextUpDaysEditor;
@override
void initState() {
super.initState();
// Initialize the controller only once.
nextUpDaysEditor = TextEditingController(
text: ref.read(clientSettingsProvider.select((value) => value.nextUpDateCutoff?.inDays ?? 14)).toString(),
);
}
@override
void dispose() {
// Dispose the controller to release resources.
nextUpDaysEditor.dispose();
super.dispose();
}
@override
Widget build(BuildContext context) {
// Build the widget tree. Replace the Container with your actual UI.
return Container(
// Example widget tree using nextUpDaysEditor.
child: Column(
children: [
TextField(controller: nextUpDaysEditor),
// ... add additional widgets as needed ...
],
),
);
}
}
After these changes, update any code that previously called buildClientSettingsVisual(...) to instead use the new ClientSettingsVisual widget. This ensures that use of the TextEditingController is managed correctly and avoids recreating it on every rebuild.
| import 'package:auto_route/auto_route.dart'; | ||
|
|
||
| @RoutePage() | ||
| class SettingsSelectionScreen extends StatelessWidget { |
Contributor
There was a problem hiding this comment.
question: Empty SettingsSelectionScreen detected.
The screen currently renders an empty SizedBox. Confirm if this is intended as a placeholder for future settings selection functionality or if additional UI should be implemented.
| import 'package:fladder/util/localization_helper.dart'; | ||
| import 'package:fladder/util/option_dialogue.dart'; | ||
|
|
||
| List<Widget> buildClientSettingsAdavanced(BuildContext context, WidgetRef ref) { |
Contributor
There was a problem hiding this comment.
issue (typo): Typo in function name: 'buildClientSettingsAdavanced'.
Renaming this function to 'buildClientSettingsAdvanced' would improve code clarity and consistency.
Suggested change
| List<Widget> buildClientSettingsAdavanced(BuildContext context, WidgetRef ref) { | |
| List<Widget> buildClientSettingsAdvanced(BuildContext context, WidgetRef ref) { |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull Request Description
This is a complete rework of the navigation stack. Fladder now no longer has separate routes for single/dual layouts. Everything is now responsive no matter the device