Add SizeTransition.fixedCrossAxisSizeFactor
#134659
Conversation
github-actions
bot
added
the
framework
|
I'm not too proud of the test. Do you have any idea how I could improve it? |
|
@Renzo-Olivares is there something I should do for this PR? |
Renzo-Olivares
left a comment
Renzo-Olivares
left a comment
There was a problem hiding this comment.
Hi @ValentinVignal thanks for the contribution! This looks awesome, I just had a few comments and suggestions for the test.
| expect(actualPositionedBox.widthFactor, 2.0); | ||
| }); | ||
|
|
||
|
|
There was a problem hiding this comment.
nit: this extra line is not needed.
There was a problem hiding this comment.
Oops, I got it removed in test: Test the size transition cross axis size
| /// | ||
| /// If the value of [crossAxisSizeFactor] is less than one, the child will be | ||
| /// clipped in the appropriate axis. | ||
| final double? crossAxisSizeFactor; |
There was a problem hiding this comment.
Maybe add a comment describing the default behavior when this is not defined. (The cross axis size is as large as the parent).
There was a problem hiding this comment.
I added a comment in chore: Add assert and comment. Is that good enough?
| /// | ||
| /// If the value of [crossAxisSizeFactor] is less than one, the child will be | ||
| /// clipped in the appropriate axis. | ||
| final double? crossAxisSizeFactor; |
There was a problem hiding this comment.
I wonder if this should have a different name since it is not directly the counter of sizeFactor since it is not an animation.
There was a problem hiding this comment.
Sure! I'm all in for a different name, I wasn't really happy with this one. Would you have a suggestion?
There was a problem hiding this comment.
maybe staticCrossAxisSizeFactor or fixedCrossAxisSizeFactor? I think its important to make it clear that the widget does not listen to this value and animate on it unlike sizeFactor.
There was a problem hiding this comment.
I decided to go with fixedCrossAxisSizeFactor and I renamed it in refactor: Rename parameter to fixedCrossAxisSizeFactor
| textDirection: TextDirection.ltr, | ||
| child: SizeTransition( | ||
| sizeFactor: animation, | ||
| crossAxisSizeFactor: 2.0, |
There was a problem hiding this comment.
This test works to verify that the crossAxisSizeFactor is actually being passed but we should also add a test that verifies the intended behavior. Maybe that test could verify the size of the parent before/after the transition when crossAxisSizeFactor is set to 1.0. Before the size would be equal to the size of the child, and after it would be 0.
There was a problem hiding this comment.
Sure! Nice idea! I added the test in test: Test the size transition cross axis size
| this.axis = Axis.vertical, | ||
| required Animation<double> sizeFactor, | ||
| this.axisAlignment = 0.0, | ||
| this.crossAxisSizeFactor, |
There was a problem hiding this comment.
we should probably assert this value is non-negative since Align cannot take a non-negative size factor.
There was a problem hiding this comment.
I copied the Align's assert in chore: Add assert and comment
…-with-size-transition # Conflicts: # packages/flutter/test/widgets/transitions_test.dart
| expect(actualPositionedBox.heightFactor, 1.0); | ||
| }); | ||
|
|
||
| testWidgetsWithLeakTracking('SizeTransition with crossAxisSizeFactor should size its cross axi from its children - vertical axis', (WidgetTester tester) async { |
There was a problem hiding this comment.
axi -> axis. Same for horizontal test.
There was a problem hiding this comment.
Oops sorry about that. I fixed it in doc: Fix typo in test description
Renzo-Olivares
left a comment
Renzo-Olivares
left a comment
There was a problem hiding this comment.
Left some comments. Thanks for making the changes!
| await tester.pump(); | ||
| expect(actualPositionedBox.heightFactor, 0.0); | ||
| expect(actualPositionedBox.widthFactor, 1.0); | ||
| expect(tester.getSize(find.byKey(key)), const Size(100, 0)); |
There was a problem hiding this comment.
I think there is an extra space in Size(100, 0).
| await tester.pump(); | ||
| expect(actualPositionedBox.heightFactor, 0.0); | ||
| expect(actualPositionedBox.widthFactor, 1.0); | ||
| expect(tester.getSize(find.byKey(key)), const Size(100, 0)); |
There was a problem hiding this comment.
Should remove this extra space here (100, 0).
| await tester.pump(); | ||
| expect(actualPositionedBox.heightFactor, 1.0); | ||
| expect(actualPositionedBox.widthFactor, 0.0); | ||
| expect(tester.getSize(find.byKey(key)), const Size(0, 100)); |
There was a problem hiding this comment.
nit: remove extra space.
| expect(tester.getSize(find.byKey(key)), const Size(100, 0)); | ||
| }); | ||
|
|
||
| testWidgetsWithLeakTracking('SizeTransition with crossAxisSizeFactor should size its cross axis from its children - horizontal axis', (WidgetTester tester) async { |
There was a problem hiding this comment.
nit: update name of crossAxisSizeFactor in tests.
…ossAxisSizeFactor
|
Oops sorry for those mistakes @Renzo-Olivares . I have fixed them in test: Better format the code and rename test description with fixedCr… |
Renzo-Olivares
left a comment
Renzo-Olivares
left a comment
There was a problem hiding this comment.
LGTM thank you for the contribution!
Renzo-Olivares
changed the title
SizeTransition.crossAxisSizeFactor SizeTransition.fixedCrossAxisSizeFactor
…-with-size-transition
Renzo-Olivares
added
the
autosubmit
auto-submit
bot
removed
the
autosubmit
3 participants
Fixes #134654
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.