-
Notifications
You must be signed in to change notification settings - Fork 3.4k
[rfw] Add Flexible widget support to core widgets #9750
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request adds support for the Flexible
widget to Remote Flutter Widgets, which is a great addition for creating more complex layouts. The implementation in core_widgets.dart
is clean and follows the existing patterns. The accompanying tests in core_widgets_test.dart
are comprehensive and cover various use cases. I have one suggestion to improve the maintainability of the new test suite by reducing code repetition. Overall, this is a solid contribution.
expect(defaultFlexible.fit, equals(FlexFit.loose)); | ||
|
||
// Test Flexible with custom flex value | ||
runtime.update(const LibraryName(<String>['test']), parseLibraryFile(''' | ||
import core; | ||
widget root = Directionality( | ||
textDirection: "ltr", | ||
child: Column( | ||
children: [ | ||
Flexible( | ||
flex: 3, | ||
child: Text(text: "Custom flex"), | ||
), | ||
], | ||
), | ||
); | ||
''')); | ||
await tester.pumpAndSettle(); | ||
final Flexible customFlexFlexible = tester.widget<Flexible>(find.byType(Flexible)); | ||
expect(customFlexFlexible.flex, equals(3)); | ||
expect(customFlexFlexible.fit, equals(FlexFit.loose)); | ||
|
||
// Test Flexible with fit: "tight" | ||
runtime.update(const LibraryName(<String>['test']), parseLibraryFile(''' | ||
import core; | ||
widget root = Directionality( | ||
textDirection: "ltr", | ||
child: Column( | ||
children: [ | ||
Flexible( | ||
flex: 2, | ||
fit: "tight", | ||
child: Text(text: "Tight fit"), | ||
), | ||
], | ||
), | ||
); | ||
''')); | ||
await tester.pumpAndSettle(); | ||
final Flexible tightFlexible = tester.widget<Flexible>(find.byType(Flexible)); | ||
expect(tightFlexible.flex, equals(2)); | ||
expect(tightFlexible.fit, equals(FlexFit.tight)); | ||
|
||
// Test Flexible with fit: "loose" | ||
runtime.update(const LibraryName(<String>['test']), parseLibraryFile(''' | ||
import core; | ||
widget root = Directionality( | ||
textDirection: "ltr", | ||
child: Column( | ||
children: [ | ||
Flexible( | ||
flex: 4, | ||
fit: "loose", | ||
child: Text(text: "Loose fit"), | ||
), | ||
], | ||
), | ||
); | ||
''')); | ||
await tester.pumpAndSettle(); | ||
final Flexible looseFlexible = tester.widget<Flexible>(find.byType(Flexible)); | ||
expect(looseFlexible.flex, equals(4)); | ||
expect(looseFlexible.fit, equals(FlexFit.loose)); | ||
|
||
// Test multiple Flexible widgets in a Column | ||
runtime.update(const LibraryName(<String>['test']), parseLibraryFile(''' | ||
import core; | ||
widget root = Directionality( | ||
textDirection: "ltr", | ||
child: Column( | ||
children: [ | ||
Flexible( | ||
flex: 1, | ||
fit: "loose", | ||
child: Text(text: "First"), | ||
), | ||
Flexible( | ||
flex: 2, | ||
fit: "tight", | ||
child: Text(text: "Second"), | ||
), | ||
Flexible( | ||
flex: 1, | ||
child: Text(text: "Third"), | ||
), | ||
], | ||
), | ||
); | ||
''')); | ||
await tester.pumpAndSettle(); | ||
expect(find.byType(Flexible), findsNWidgets(3)); | ||
|
||
final List<Flexible> flexibleWidgets = tester.widgetList<Flexible>(find.byType(Flexible)).toList(); | ||
expect(flexibleWidgets[0].flex, equals(1)); | ||
expect(flexibleWidgets[0].fit, equals(FlexFit.loose)); | ||
expect(flexibleWidgets[1].flex, equals(2)); | ||
expect(flexibleWidgets[1].fit, equals(FlexFit.tight)); | ||
expect(flexibleWidgets[2].flex, equals(1)); | ||
expect(flexibleWidgets[2].fit, equals(FlexFit.loose)); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test cases for the Flexible
widget are comprehensive, but the implementation is quite repetitive. Each scenario re-defines the entire widget root
and has similar boilerplate code.
To improve readability and maintainability, you could refactor this by extracting the common test logic into a helper function. This would make the test suite more concise and easier to extend in the future.
Here's an example of what a helper function could look like:
Future<void> _testFlexible(
WidgetTester tester,
Runtime runtime, {
required String rfwSnippet,
required void Function(Flexible flexible) verifier,
}) async {
runtime.update(const LibraryName(<String>['test']), parseLibraryFile('''
import core;
widget root = Directionality(
textDirection: "ltr",
child: Column(
children: [
$rfwSnippet
],
),
);
'''));
await tester.pumpAndSettle();
verifier(tester.widget(find.byType(Flexible)));
}
Each test case would then become a much simpler call to this helper.
Thanks for the contribution! You’ve checked boxes in the PR checklist above that are not reflected in this PR, so I’m assuming this is a work in progress and am marking it as a Draft. Please review the checklist, updating the PR as appropriate, and when the state of the PR as posted reflects the checklist please feel free to mark it as ready for review.
Please read the documentation linked from the entry you checked here.
Please read the exemption documention linked from the entries you checked here. |
81fe452
to
80a77cf
Compare
@stuartmorgan-g ✅ Issues Resolved:1. Issue Linking Fixed
2. Version Management Corrected
3. Checklist Accuracy Verified
🔧 Additional Improvements Made:
📋 Current PR State:
I apologize for the initial checklist inconsistencies and appreciate your patience in helping me understand Flutter's Thank you for the guidance! |
[rfw] Add Flexible widget support
This PR adds support for the
Flexible
widget to Remote Flutter Widgets (RFW), allowing developers to create flexible layouts with both loose and tight fitting behavior.Fixes #173313
The
Flexible
widget provides more granular control over flex layouts compared to the existingExpanded
widget:Implementation:
Flexible
widget tolib/src/flutter/core_widgets.dart
flex
(default: 1) andfit
parameters (loose/tight, default: loose)ArgumentDecoders.enumValue
pattern for FlexFit parsingtest/core_widgets_test.dart
Usage Example:
API Compatibility:
Follows Flutter's
Flexible
widget API exactly:flex: int
(default: 1)fit: FlexFit
(loose/tight, default: loose)child: Widget
(required)Pre-Review Checklist
[rfw]
pubspec.yaml
with an appropriate new version according to the pub versioning philosophy.CHANGELOG.md
to add a description of the change, following repository CHANGELOG style.///
).If you need help, consider asking for advice on the #hackers-new channel on Discord.