Added OverflowBar widget #62350

HansMuller · 2020-07-27T18:37:17Z

This widget provides most of the same features as ButtonBar, without being tangled up with the Material ButtonTheme.

DartPad demo of OverflowBar: https://dartpad.dev/4791c69a962dd9da97b7b9748ac2983c

rami-a

LGTM

rami-a · 2020-07-27T20:29:28Z

packages/flutter/lib/src/widgets/overflow_bar.dart

+///       color: Colors.white,
+///       elevation: 24,
+///       shape: RoundedRectangleBorder(
+///         borderRadius: BorderRadius.all(Radius.circular(4.0))


Nit: Demo code should either consistently use or omit the .0 on doubles

packages/flutter/lib/src/widgets/overflow_bar.dart

rami-a · 2020-07-27T20:35:47Z

packages/flutter/lib/src/widgets/overflow_bar.dart

+  /// [OverflowBarAlignment.start], and with the left edge of the
+  /// available space for [OverflowBarAlignment.end]. For
+  /// [OverflowBarAlignment.center] each child is horizontally
+  /// centered with the available space.


Suggested change

/// centered with the available space.

/// centered within the available space.

maybe?

rami-a · 2020-07-27T20:38:17Z

packages/flutter/lib/src/widgets/overflow_bar.dart

+  ///    of the children within the vertical "overflow" layout.
+  final VerticalDirection overflowDirection;
+
+  /// Determines the order that the [children] appear in for the default


If this is null, then it uses the inherited text direction right? Probably worth mentioning that in the documentation.

rami-a · 2020-07-27T20:39:10Z

packages/flutter/lib/src/widgets/overflow_bar.dart

+  void debugFillProperties(DiagnosticPropertiesBuilder properties) {
+    super.debugFillProperties(properties);
+    properties.add(DoubleProperty('spacing', spacing, defaultValue: 0));
+    properties.add(DoubleProperty('overflowSpacing', overflowSpacing));


To match the other lines

Suggested change

properties.add(DoubleProperty('overflowSpacing', overflowSpacing));

properties.add(DoubleProperty('overflowSpacing', overflowSpacing, defaultValue: 0));

rami-a · 2020-07-27T20:41:47Z

packages/flutter/lib/src/widgets/overflow_bar.dart

+      child = childAfter(child);
+    }
+
+    final bool rtl = textDirection == TextDirection.rtl;


Nit:

Suggested change

final bool rtl = textDirection == TextDirection.rtl;

final bool isRtl = textDirection == TextDirection.rtl;

I had it this way originally but I suffer from a personal loathing for logically capitalizing acronyms, so rtl.

rami-a · 2020-07-27T20:42:39Z

packages/flutter/lib/src/widgets/overflow_bar.dart

+    } else {
+      // Default horizontal layout
+      child = rtl ? lastChild : firstChild;
+      RenderBox nextChild() => textDirection == TextDirection.rtl ? childBefore(child) : childAfter(child);


Suggested change

RenderBox nextChild() => textDirection == TextDirection.rtl ? childBefore(child) : childAfter(child);

RenderBox nextChild() => rtl ? childBefore(child) : childAfter(child);

rami-a · 2020-07-27T20:44:21Z

packages/flutter/test/widgets/overflow_bar_test.dart

+    expect(bar.children, const <Widget>[]);
+  });
+
+  testWidgets('EmptyOverflowBar', (WidgetTester tester) async {


Nit/Suggestion:

Suggested change

testWidgets('EmptyOverflowBar', (WidgetTester tester) async {

testWidgets('Empty OverflowBar', (WidgetTester tester) async {

rami-a · 2020-07-27T20:45:39Z

packages/flutter/test/widgets/overflow_bar_test.dart

+    // Children are vertically centered, start at x=0
+    await tester.pumpWidget(buildFrame(spacing: 10, textDirection: TextDirection.ltr));
+    expect(tester.getRect(find.byKey(child1Key)), const Rect.fromLTRB(0, 8, 48, 56));
+    expect(tester.getRect(find.byKey(child2Key)), const Rect.fromLTRB(10.0 + 48, 0, 10.0 + 112, 64));


Nit: Should probably be consistent with keeping/omitting the .0 for doubles.

I find that omitting the .0 makes code easier to read however it was necessary to introduce one in these expressions so that they eval to doubles instead of ints. I realize that it's inconsistent but I've tried to be consistent about that.

rami-a · 2020-07-27T20:46:15Z

packages/flutter/test/widgets/overflow_bar_test.dart

+    expect(tester.getRect(find.byKey(child1Key)), const Rect.fromLTRB(100.0/2.0 - 48/2, 0, 100.0/2.0 + 48/2, 48));
+    expect(tester.getRect(find.byKey(child2Key)), const Rect.fromLTRB(100.0/2.0 - 64/2, 48, 100.0/2.0 + 64/2, 112));
+    expect(tester.getRect(find.byKey(child3Key)), const Rect.fromLTRB(100.0/2.0 - 32/2, 112, 100.0/2.0 + 32/2, 144));
+


Nit: Unnecessary new line

darrenaustin

Nice! LGTM, just a question about defaults.

darrenaustin · 2020-07-27T22:20:35Z

packages/flutter/lib/src/widgets/overflow_bar.dart

+  /// used instead.
+  ///
+  /// Defaults to 0.0.
+  final double spacing;


I wonder if we should make this default match the material spec for button spacing? Or is that assuming too much? It would just be nice to just wrap an array of buttons with an OverflowBar with no params and have it match the old ButtonBar layout by default.

For a dialog you'd also need to end-align the OverflowBar and somewhere you need to ensure that the OverflowBar can be scrolled into view.

Using 0 for the spacing seemed appropriate for the widgets library since it's probably the wrong place to have an opinion about spacing.

Ah, I missed that it was part of the widgets library 😄. Totally makes sense for it to be there and that the default should be 0.

goderbauer · 2020-07-28T19:28:40Z

packages/flutter/lib/src/widgets/overflow_bar.dart

+///
+/// This example defines a simple approximation of a dialog
+/// layout, where the layout of the dialog's action buttons are
+/// defined by an [OverflowBar].  The content is wrapped in a


nit: double space

goderbauer · 2020-07-28T19:28:54Z

packages/flutter/lib/src/widgets/overflow_bar.dart

+/// layout, where the layout of the dialog's action buttons are
+/// defined by an [OverflowBar].  The content is wrapped in a
+/// [SingleChildScrollView], so that if overflow occurs, the
+/// action buttons will be still be accessible by scrolling,


... will be still be ... ?

goderbauer · 2020-07-28T19:30:12Z

packages/flutter/lib/src/widgets/overflow_bar.dart

+class OverflowBar extends MultiChildRenderObjectWidget {
+  /// Constructs an OverflowBar.
+  ///
+  /// The [spacing], [overflowSpacing], [overflowAlignment],


I believe children of an MultiChildRenderObjectWidget also cannot be null. Mention that in the docs and add an assert?

goderbauer · 2020-07-28T20:05:28Z

packages/flutter/lib/src/widgets/overflow_bar.dart

+
+  @override
+  double computeMinIntrinsicHeight(double width) {
+    RenderBox child = firstChild;


Is this calculation correct? I'd assumed that if width is big enough everything gets layed out in a row and then the hight would not be the sum of the children heights, no?

Yes, you are correct, this method is incorrect. The min intrinsic height should be: if the sum of child intrinsic widths fit, then the max child height, otherwise the sum of the child heights.

I've fixed this however I still need to add some tests for min/max intrinsic height.

fluttergithubbot added the framework flutter/packages/flutter repository. See also f: labels. label Jul 27, 2020

googlebot added the cla: yes label Jul 27, 2020

HansMuller requested review from rami-a and darrenaustin July 27, 2020 20:12

rami-a approved these changes Jul 27, 2020

View reviewed changes

darrenaustin approved these changes Jul 27, 2020

View reviewed changes

goderbauer reviewed Jul 28, 2020

View reviewed changes

Hans Muller added 4 commits July 28, 2020 13:07

Added OverflowBar widget

9b4e2d2

Updated per review feedfback

9fe6e46

Corrected imports, removed material.dart import

65a22cb

updated per review feedback

ac7217d

HansMuller force-pushed the overflow_bar branch from ef06b1d to ac7217d Compare July 28, 2020 20:30

Corrected support for intrinsic width/height methods per review feedback

cd0ee57

HansMuller added the waiting for tree to go green label Jul 29, 2020

fluttergithubbot merged commit 6139708 into flutter:master Jul 29, 2020

This was referenced Jul 29, 2020

_RenderButtonBarRow issues #53378

Open

Breaking Change Proposal: ButtonBar's buttonPadding's vertical padding #48606

Open

ButtonBar applies it's own theme #51144

Closed

Pragya007 pushed a commit to Pragya007/flutter that referenced this pull request Aug 11, 2020

Added OverflowBar widget (flutter#62350)

b643901

mingwandroid pushed a commit to mingwandroid/flutter that referenced this pull request Sep 6, 2020

Added OverflowBar widget (flutter#62350)

5db4b56

HansMuller deleted the overflow_bar branch June 26, 2021 00:11

	/// centered with the available space.
	/// centered within the available space.

	properties.add(DoubleProperty('overflowSpacing', overflowSpacing));
	properties.add(DoubleProperty('overflowSpacing', overflowSpacing, defaultValue: 0));

	final bool rtl = textDirection == TextDirection.rtl;
	final bool isRtl = textDirection == TextDirection.rtl;

	RenderBox nextChild() => textDirection == TextDirection.rtl ? childBefore(child) : childAfter(child);
	RenderBox nextChild() => rtl ? childBefore(child) : childAfter(child);

	testWidgets('EmptyOverflowBar', (WidgetTester tester) async {
	testWidgets('Empty OverflowBar', (WidgetTester tester) async {

Added OverflowBar widget #62350

Added OverflowBar widget #62350

Uh oh!

Conversation

HansMuller commented Jul 27, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rami-a 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!

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!

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!

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!

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!

darrenaustin 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!

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!

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!

HansMuller commented Jul 27, 2020 •

edited

Loading