Configurable padding for the CupertinoSegmentedControl #30807

vanlooverenkoen · 2019-04-09T20:03:55Z

Added a configurable padding for the CupertinoSegmentedControl. The default of 16 will still be used if no padding was set.

#25989

LongCatIsLooong · 2019-04-15T18:49:51Z

Hi @vanlooverenkoen, thanks for your contribution! Could you add some tests for the changes you made?

…lso the custom padding is set

vanlooverenkoen · 2019-05-03T17:01:44Z

I have added 2 tests

1 to make sure the default padding is shown
1 to make sure the custom padding is shown.

…-segmented-control

vanlooverenkoen · 2019-05-09T15:29:42Z

@LongCatIsLooong, is there something else I should do to get this PR merged?

LongCatIsLooong

Sorry for taking so long.

LongCatIsLooong · 2019-05-09T22:34:01Z

packages/flutter/lib/src/cupertino/segmented_control.dart

  /// Defaults to the selectedColor at 20% opacity if null.
  final Color pressedColor;

+  /// The [children], if any, are placed inside this padding.


It's guaranteed a segmented control will have more than 1 child.

flutter/packages/flutter/lib/src/cupertino/segmented_control.dart

Lines 91 to 92 in 0de21c0

}) : assert(children != null),

assert(children.length >= 2),

LongCatIsLooong · 2019-05-09T23:01:41Z

packages/flutter/test/cupertino/segmented_control_test.dart

+      ),
+    );
+
+    expect(find.byType(Padding), findsOneWidget);


Inspecting Padding directly to see if the parameter is set correctly seems a bit implementation dependent, IMO it's better to verify the expected behavior.

I'd prefer to verify that the bounding Rect of child widgets is indeed inset by padding amount, i.e. verify the top left corner of "Child 1" is Offset(16, 0) to the top left corner of CupertinoSegmentedControl<int> etc..

How should I implement this test? coordinates_test.dart is what I am looking at at this point.

final RenderBox boxA = tester.renderObject(find.byKey(keyChild1)); expect(boxA.globalToLocal(Offset.zero), equals(const Offset(0.0, 286.0)));

How can I check what the position is between 2 widgets?

The WidgetTester class offers methods to get the key coordinates of a widget, e.g. getBottomLeft.
A good example would be this test:

flutter/packages/flutter/test/cupertino/segmented_control_test.dart

Lines 374 to 399 in 0de21c0

testWidgets('Widgets are centered within segments', (WidgetTester tester) async {

final Map<int, Widget> children = <int, Widget>{};

children[0] = const Text('Child 1');

children[1] = const Text('Child 2');

await tester.pumpWidget(

Directionality(

textDirection: TextDirection.ltr,

child: Align(

alignment: Alignment.topLeft,

child: SizedBox(

width: 200.0,

height: 200.0,

child: CupertinoSegmentedControl<int>(

children: children,

onValueChanged: (int newValue) { },

),

),

),

),

);

// Widgets are centered taking into account 16px of horizontal padding

expect(tester.getCenter(find.text('Child 1')), const Offset(58.0, 100.0));

expect(tester.getCenter(find.text('Child 2')), const Offset(142.0, 100.0));

});

e.g.

testWidgets('Padding works', (WidgetTester tester) async { const Key key = Key('Container'); final Map<int, Widget> children = <int, Widget>{}; children[0] = const SizedBox( height: double.infinity, child: Text('Child 1'), ) ; children[1] = const SizedBox( height: double.infinity, child: Text('Child 2'), ) ; Future<void> verifyPadding({ EdgeInsets padding }) async { final EdgeInsets effectivePadding = padding ?? const EdgeInsets.symmetric(horizontal: 16); final Rect segmentedControlRect = tester.getRect(find.byKey(key)); expect( tester.getTopLeft(find.byWidget(children[0])), segmentedControlRect.topLeft.translate( effectivePadding.topLeft.dx, effectivePadding.topLeft.dy, ) ); expect( tester.getBottomLeft(find.byWidget(children[0])), segmentedControlRect.bottomLeft.translate( effectivePadding.bottomLeft.dx, effectivePadding.bottomLeft.dy, ), ); expect( tester.getTopRight(find.byWidget(children[1])), segmentedControlRect.topRight.translate( effectivePadding.topRight.dx, effectivePadding.topRight.dy, ), ); expect( tester.getBottomRight(find.byWidget(children[1])), segmentedControlRect.bottomRight.translate( effectivePadding.bottomRight.dx, effectivePadding.bottomRight.dy, ), ); } await tester.pumpWidget( boilerplate( child: CupertinoSegmentedControl<int>( key: key, children: children, onValueChanged: (int newValue) { }, ), ) ); // Default padding works. await verifyPadding(); // Switch to Child 2 padding should remain the same. await tester.tap(find.text('Child 2')); await tester.pumpAndSettle(); await verifyPadding(); await tester.pumpWidget( boilerplate( child: CupertinoSegmentedControl<int>( key: key, padding: const EdgeInsets.fromLTRB(1, 3, 5, 7), children: children, onValueChanged: (int newValue) { }, ), ) ); // Custom padding works. await verifyPadding(padding: const EdgeInsets.fromLTRB(1, 3, 5, 7)); // Switch back to Child 1 padding should remain the same. await tester.tap(find.text('Child 1')); await tester.pumpAndSettle(); await verifyPadding(padding: const EdgeInsets.fromLTRB(1, 3, 5, 7)); });

LongCatIsLooong · 2019-05-09T23:01:54Z

packages/flutter/test/cupertino/segmented_control_test.dart

+      ),
+    );
+
+    expect(find.byType(Padding), findsOneWidget);


xster · 2019-05-14T22:10:34Z

packages/flutter/lib/src/cupertino/segmented_control.dart

+  /// The [children], if any, are placed inside this padding.
+  ///
+  /// Defaults to EdgeInsets.symmetric(horizontal: 16.0)
+  final EdgeInsets padding;


Let's change this to EdgeInsetsGeometry. Otherwise this is specifying that we use left and right regardless of Directionality.

packages/flutter/lib/src/cupertino/segmented_control.dart

xster · 2019-05-14T22:11:26Z

packages/flutter/lib/src/cupertino/segmented_control.dart


    return Padding(
-      padding: _kHorizontalItemPadding.resolve(Directionality.of(context)),
+      padding: widget.padding ?? _kHorizontalItemPadding.resolve(Directionality.of(context)),


Once we change it to an EdgeInsetGeometry, we should remove the .resolve here.

vanlooverenkoen · 2019-06-03T09:12:23Z

@LongCatIsLooong
How should I implement this test? coordinates_test.dart is what I am looking at at this point.

    final RenderBox boxA = tester.renderObject(find.byKey(keyChild1));
    expect(boxA.globalToLocal(Offset.zero), equals(const Offset(0.0, 286.0)));

How can I check what the position is between 2 widgets?

LongCatIsLooong · 2019-06-04T21:34:51Z

@vanlooverenkoen I updated the comment with an example.

…ed behavior instead of the implementation

LongCatIsLooong

LGTM

LongCatIsLooong · 2019-06-06T03:13:11Z

packages/flutter/lib/src/cupertino/segmented_control.dart

  /// Defaults to the selectedColor at 20% opacity if null.
  final Color pressedColor;

+  /// The CupertinoSegmentedControl will be placed inside this padding


Copied from the docs of _kHorizontalItemPadding

Suggested change

/// The CupertinoSegmentedControl will be placed inside this padding

/// Minimum padding from edges of the segmented control to edges of encompassing

/// widget.

Sorry what I was talking about the padding property in CupertinoSegmentedControl. The padding is part of the CupertinoSegmentedControl widget but it sounded like it is not.

…-segmented-control

LongCatIsLooong · 2019-06-13T01:42:30Z

@vanlooverenkoen could you merge the current master branch (from upstream) into this PR? There's something weird happening with the flutter SDK version.

…-segmented-control

vanlooverenkoen · 2019-06-13T16:14:32Z

@LongCatIsLooong done

vanlooverenkoen · 2019-06-13T16:39:47Z

😑

vanlooverenkoen · 2019-06-13T16:47:12Z

I cannot build anymore :(

Because every version of flutter_goldens from sdk depends on pedantic 1.5.0 and flutter depends on pedantic 1.7.0, flutter_goldens from sdk is forbidden.

So, because flutter depends on flutter_goldens any from sdk, version solving failed.
pub get failed (1)

LongCatIsLooong · 2019-06-13T20:09:49Z

@vanlooverenkoen sorry for the inconvenience but could you rebase master and squash all your commits into one? Apparently if you merge the latest master into your branch the SDK version is still decided by the commit you branched off from master when you created this feature branch.

…-segmented-control

vanlooverenkoen · 2019-06-14T08:05:42Z

😑 I do not understand what I am doing wrong

LongCatIsLooong · 2019-06-14T17:31:20Z

My apologies for the inconvenience 😞, the versioning system in flutter SDK does not handle merge commits by design. Could you do a git rebase upstream/master, to squash all your commits into one, and force push it to this branch? Or if you don't mind I can open a new PR with your changes.

vanlooverenkoen · 2019-06-16T09:04:22Z

@LongCatIsLooong #34555

googlebot added the cla: yes label Apr 9, 2019

goderbauer added f: cupertino flutter/packages/flutter/cupertino repository framework flutter/packages/flutter repository. See also f: labels. labels Apr 9, 2019

#25989: Added padding. default will still be the 16 horizontal padding

d259095

#25989: Added some test to make sure the default padding is set but a…

2f56889

…lso the custom padding is set

Merge branch 'master' into Feature/25989-horizontal-padding-cupertino…

0de21c0

…-segmented-control

xster assigned LongCatIsLooong May 9, 2019

LongCatIsLooong requested changes May 9, 2019

View reviewed changes

HansMuller changed the title ~~#25989: Added padding. default will still be the 16 horizontal padding~~ Configurable padding for the CupertinoSegmentedControl May 14, 2019

xster reviewed May 14, 2019

View reviewed changes

LongCatIsLooong added the waiting for customer response The Flutter team cannot make further progress on this issue until the original reporter responds label May 30, 2019

HansMuller removed the waiting for customer response The Flutter team cannot make further progress on this issue until the original reporter responds label Jun 4, 2019

#25989: Fixed pull request comments. Changed test to test the request…

22fcc88

…ed behavior instead of the implementation

LongCatIsLooong approved these changes Jun 6, 2019

View reviewed changes

LongCatIsLooong reviewed Jun 6, 2019

View reviewed changes

vanlooverenkoen added 2 commits June 8, 2019 11:24

#25989 Fixed pull request feedback.

9604797

Merge branch 'master' into Feature/25989-horizontal-padding-cupertino…

53aac23

…-segmented-control

Merge branch 'master' into Feature/25989-horizontal-padding-cupertino…

9848070

…-segmented-control

Merge branch 'master' into Feature/25989-horizontal-padding-cupertino…

80cf3c2

…-segmented-control

vanlooverenkoen mentioned this pull request Jun 16, 2019

Added customizable padding for the segmented controll #34555

Merged

vanlooverenkoen closed this Jun 16, 2019

vanlooverenkoen deleted the Feature/25989-horizontal-padding-cupertino-segmented-control branch June 17, 2019 07:50

github-actions bot locked as resolved and limited conversation to collaborators Aug 6, 2021

	testWidgets('Widgets are centered within segments', (WidgetTester tester) async {
	final Map<int, Widget> children = <int, Widget>{};
	children[0] = const Text('Child 1');
	children[1] = const Text('Child 2');

	await tester.pumpWidget(
	Directionality(
	textDirection: TextDirection.ltr,
	child: Align(
	alignment: Alignment.topLeft,
	child: SizedBox(
	width: 200.0,
	height: 200.0,
	child: CupertinoSegmentedControl<int>(
	children: children,
	onValueChanged: (int newValue) { },
	),
	),
	),
	),
	);

	// Widgets are centered taking into account 16px of horizontal padding
	expect(tester.getCenter(find.text('Child 1')), const Offset(58.0, 100.0));
	expect(tester.getCenter(find.text('Child 2')), const Offset(142.0, 100.0));
	});

	/// The CupertinoSegmentedControl will be placed inside this padding
	/// Minimum padding from edges of the segmented control to edges of encompassing
	/// widget.

Configurable padding for the CupertinoSegmentedControl #30807

Configurable padding for the CupertinoSegmentedControl #30807

Uh oh!

Conversation

vanlooverenkoen commented Apr 9, 2019

Uh oh!

LongCatIsLooong commented Apr 15, 2019

Uh oh!

vanlooverenkoen commented May 3, 2019

Uh oh!

vanlooverenkoen commented May 9, 2019

Uh oh!

LongCatIsLooong left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vanlooverenkoen May 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LongCatIsLooong Jun 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vanlooverenkoen commented Jun 3, 2019

Uh oh!

LongCatIsLooong commented Jun 4, 2019

Uh oh!

LongCatIsLooong left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LongCatIsLooong commented Jun 13, 2019

Uh oh!

vanlooverenkoen commented Jun 13, 2019

Uh oh!

vanlooverenkoen commented Jun 13, 2019

Uh oh!

vanlooverenkoen commented Jun 13, 2019

Uh oh!

LongCatIsLooong commented Jun 13, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vanlooverenkoen commented Jun 14, 2019

Uh oh!

LongCatIsLooong commented Jun 14, 2019

Uh oh!

vanlooverenkoen commented Jun 16, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

vanlooverenkoen May 26, 2019 •

edited

Loading

LongCatIsLooong Jun 3, 2019 •

edited

Loading

LongCatIsLooong commented Jun 13, 2019 •

edited

Loading