Add parameters to allow hiding icons and border of the Cupertino TabBar #22804

artur-ios-dev · 2018-10-08T11:06:17Z

This PR introduces two new parameters for the CupertinoTabBar.

onlyIcons which enables only-icons mode by hiding title of all the items
hideBorder - if true hides the top border of the tab bar.

xster · 2018-10-09T19:02:04Z

Thanks for your contribution. Thanks so much for adding tests as well!

I have 2 general design level feedback if you're willing to iterate on this PR a bit more:

Instead of a specific feature to hide the borders, just let there be a default Border as a construction parameter but that lets the user override it.
We should fix the optional title issue higher up the stream. Let BottomNavigationBarItem.title be nullable and assert it to be non-null in the Material BottomNavigationBar's constructor (since Material spec doesn't allow un-labeled tabs).

artur-ios-dev · 2018-10-09T19:52:50Z

@xster Reasonable and great points. Will do that!

artur-ios-dev · 2018-10-10T08:19:46Z

@xster Ready to review again 📃

xster

This is great. Exactly what I was hoping. Thanks for making the changes. Just some last comments.

xster · 2018-10-10T18:09:49Z

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

+        style: BorderStyle.solid,
+      ),
+    ),
+  })  : assert(items != null),


detail: remove double space before : and indent everything back by one.

xster · 2018-10-10T18:11:42Z

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

+
  /// Change the active tab item's icon and title colors to active.
-  Widget _wrapActiveItem(Widget item, { @required bool active }) {
+  Widget _wrapActiveItem(Widget item, {@required bool active}) {


was this from dartfmt?

I use Formatter included with Visual Studio Code - is this the same though? It added "double space" from the command above as well 🤔

xster · 2018-10-10T18:13:01Z

packages/flutter/lib/src/material/bottom_navigation_bar.dart

    this.iconSize = 24.0,
  }) : assert(items != null),
       assert(items.length >= 2),
+       assert(items.every((BottomNavigationBarItem item) => item.title != null) == true, "Each item's title should not be null"),


Describe this requirement on lines 80 and 105 too.

This line is too long.

assert( ..., "Each...", ),

xster · 2018-10-10T18:15:39Z

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

  /// Creates an item that is used with [BottomNavigationBar.items].
  ///
-  /// The arguments [icon] and [title] should not be null.
+  /// The argument [icon] should not be null and the argument [title] should not be null for [BottomNavigationBar] parent.


and the argument [title] should not be null when used in a Material Design's [BottomNavigationBar].

xster · 2018-10-10T18:15:51Z

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

  const BottomNavigationBarItem({
    @required this.icon,
-    @required this.title,
+    this.title,


Describe in line 64 too.

xster · 2018-10-10T18:20:16Z

packages/flutter/test/cupertino/bottom_tab_bar_test.dart

+        ));
+
+    expect(find.text('Tab 1'), findsOneWidget);
+    expect(find.text('Tab 2'), findsNothing);


Negative tests that purely checks that something doesn't happen doesn't add much value. You'd want to test that the image only second tab button did appear ok. See 'Use active icon' above for an example referencing an image to assert that it shows up.

Looking at this example and I am not sure how I can find the image on the second Tab since there is no text next to it 🤔

ah, you can use find.byWidgetPredicate to find a widget with a particular TestImageProvider instance. You can also then click the finder with tester.tap to make sure it still works.

xster · 2018-10-10T18:22:48Z

packages/flutter/test/cupertino/bottom_tab_bar_test.dart

+
+    final DecoratedBox decoratedBox = tester.widget(find.byType(DecoratedBox));
+    final BoxDecoration boxDecoration = decoratedBox.decoration;
+    expect(boxDecoration.border != null, equals(true));


You can just write this as expect(boxDecoration.border, isNotNull/isNull);

Great, that's what I was looking for :)

xster · 2018-10-10T18:23:44Z

packages/flutter/test/material/bottom_navigation_bar_test.dart

+                  type: BottomNavigationBarType.shifting,
+                  items: const <BottomNavigationBarItem>[
+            BottomNavigationBarItem(
+                icon: Icon(Icons.ac_unit), title: Text('AC')),


Adding trailing commas after every closing parenthesis/brackets will keep dartfmt properly formatted.

Oh, I keep forgetting about that although the formatter acts really weird sometimes 😭 and I am really getting lost with that - changing one thing and half of the file changes 😨

artur-ios-dev · 2018-10-10T20:19:17Z

Thank you for your review. Fixed (hopefully) almost all things you mentioned. Although files formatting is getting out of hands... kinda 😖 It does weird things. Tried using the built one into VSC and it does the same thing as flutter format.

Shall I revert all the changes made by the code formatter except my additions?

xster

Thanks for your patience

xster · 2018-10-10T21:13:03Z

packages/flutter/lib/src/material/bottom_navigation_bar.dart

    this.iconSize = 24.0,
  }) : assert(items != null),
       assert(items.length >= 2),
+       assert(


sorry I'm just nitpicking at this point :)
just to keep our formatting consistent:

assert( items.every((BottomNavigationBarItem item) => item.title != null) == true, "Every item must have a non-null title", ),

xster · 2018-10-11T06:09:15Z

packages/flutter/lib/src/material/bottom_navigation_bar.dart

-      );
+          parent: _controllers[index],
+          curve: Curves.fastOutSlowIn,
+          reverseCurve: Curves.fastOutSlowIn.flipped);


nit: this one got formatted weird too because there's no trailing comma

xster · 2018-10-11T06:11:06Z

packages/flutter/lib/src/material/bottom_navigation_bar.dart

-  }) : assert(state != null),
-       assert(index != null),
-       assert(color != null) {
+  })  : assert(state != null),


bring this a space back

xster · 2018-10-11T06:11:25Z

packages/flutter/lib/src/material/bottom_navigation_bar.dart


    // Add half of its flex value in order to get to the center.
-    return (leadingWeights + state._evaluateFlex(state._animations[index]) / 2.0) / allWeights;
+    return (leadingWeights +


this one turned a bit strange. I'd leave this in one line

xster · 2018-10-11T06:11:31Z

packages/flutter/lib/src/material/bottom_navigation_bar.dart

    @required this.textDirection,
-  }) : assert(circles != null),
-       assert(textDirection != null);
+  })  : assert(circles != null),


xster · 2018-10-11T06:12:37Z

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

  final Widget activeIcon;

-  /// The title of the item.
+  /// The title of the item. If the title is not provided only the icon will be shown.


shown when not used in a Material Design [BottomNavigationBar].

xster · 2018-10-11T06:19:02Z

packages/flutter/test/cupertino/bottom_tab_bar_test.dart

+                title: Text('Tab 1'),
+              ),
+              BottomNavigationBarItem(
+                  icon: ImageIcon(TestImageProvider(24, 24))),


nit: trailing comma

xster · 2018-10-11T06:20:06Z

packages/flutter/test/cupertino/bottom_tab_bar_test.dart

+        ));
+
+    expect(find.text('Tab 1'), findsOneWidget);
+    expect(find.text('Tab 2'), findsNothing);


ah, you can use find.byWidgetPredicate to find a widget with a particular TestImageProvider instance. You can also then click the finder with tester.tap to make sure it still works.

artur-ios-dev · 2018-10-11T07:47:06Z

Ah, that auto formatter... Gotta pay more attention to that. Improved the test as well as you suggested.

xster

Thanks. One last thing before I merge it in.

xster · 2018-10-11T15:49:10Z

packages/flutter/test/material/bottom_navigation_bar_test.dart

-      )
-    );
+            bottomNavigationBar:
+                BottomNavigationBar(items: const <BottomNavigationBarItem>[


Unfortunately a bunch of these turned out a bit wonky. Either revert the auto-format and just keep the change to the addition of the new test or (expanding the scope a bit unfortunately) add trailing commas so the formatter keeps the general shape cascading.

artur-ios-dev · 2018-10-12T06:49:43Z

@xster Reverted that auto-format changes. Might just create another PR that fixes formatting (if we want that globally) later on.

xster · 2018-10-12T16:08:51Z

Thanks, merging

…r/engine#22816)

* d6beaed Roll Fuchsia Linux SDK from gkfmiRsIl... to un3JixwuO... (flutter/engine#22744) * 8832b48 Roll Skia from 888c5d3e57eb to 51b74afb84d4 (12 revisions) (flutter/engine#22746) * e890901 Don't register CanvasKit with `define` (flutter/engine#22745) * 3c51679 Roll Skia from 51b74afb84d4 to 452369182f6e (1 revision) (flutter/engine#22749) * 5bf6533 Introduce a delegate class for gpu metal rendering (flutter/engine#22611) * 5131aa4 Roll Skia from 452369182f6e to f2efb80bc316 (4 revisions) (flutter/engine#22750) * 7b5f79f fuchsia: Ensure full-screen input interceptor (flutter/engine#22687) * cec8a6e Manual roll of Dart SDK from ce76503f5b46 to dcd5a8f005a (flutter/engine#22766) * 001a511 Roll Fuchsia Linux SDK from un3JixwuO... to Bnaeivv07... (flutter/engine#22757) * b9615b1 Roll Fuchsia Mac SDK from 36uDTGJQp... to qpkZl0s5J... (flutter/engine#22753) * c4c4763 Roll Skia from f2efb80bc316 to 8d78da910e45 (5 revisions) (flutter/engine#22754) * dbd1abe Roll Dart SDK from dcd5a8f005a2 to 960620d2e811 (794 revisions) (flutter/engine#22768) * 1c2a6bd Fix the unchecked conversion warning for searchPaths in PlayStoreDynamicFeatureManager (flutter/engine#22654) * 81af789 add file package to deps in prep for glob update (flutter/engine#22770) * a35e3fe Let FlutterFragment not pop the whole activity by default when more fragments are in the activity (flutter/engine#22692) * adb3312 Revert "Introduce a delegate class for gpu metal rendering (#22611)" (flutter/engine#22775) * bcc8832 Cleanup dart_runner examples & tests. (flutter/engine#22769) * 609307d Roll Skia from 8d78da910e45 to fd41d878b13d (20 revisions) (flutter/engine#22772) * 587c023 [web] Add new line break type (prohibited) (flutter/engine#22771) * 6b2ed2b Roll Skia from fd41d878b13d to 70fe17e12f38 (6 revisions) (flutter/engine#22776) * 7910a17 Roll Dart SDK from 960620d2e811 to 7a2a3968ef53 (12 revisions) (flutter/engine#22778) * f4ada80 Roll Skia from 70fe17e12f38 to 4c6f57a23e63 (1 revision) (flutter/engine#22781) * 3101dff [web] Optimize Matrix4.identity (flutter/engine#22622) * a4ce848 Add FlutterPlayStoreSplitApplication for simpler opt in to Split AOT (flutter/engine#22752) * 747b791 Add file.dart to DEPS (flutter/engine#22794) * 40fa345 Fix race condition in key event handling on Android (flutter/engine#22658) * d2ad441 Fix PlatformDispatcher.locale to return something meaningful when there are no locales. (flutter/engine#22608) * b9a0b5e Roll Skia from 4c6f57a23e63 to a927771c9cce (10 revisions) (flutter/engine#22802) * 96d63e5 Roll Dart SDK from 7a2a3968ef53 to e9a03fd98faa (5 revisions) (flutter/engine#22801) * cdf72da Roll Skia from a927771c9cce to 7b776b514933 (3 revisions) (flutter/engine#22803) * a0c8b67 Roll buildroot and benchmark (flutter/engine#22804) * c3c3ec6 Roll Fuchsia Mac SDK from qpkZl0s5J... to 7O11wjLVX... (flutter/engine#22805) * 6625308 Revert "Roll buildroot and benchmark (#22804)" (flutter/engine#22816) * 64d9add Add a golden scenario test for fallback font rendering on iOS take 3 (flutter/engine#22736) * 7d7a260 Add static text trait to plain semantics object with label in iOS (flutter/engine#22811) * 22e1143 Roll Skia from 7b776b514933 to c504ecda03b8 (6 revisions) (flutter/engine#22808) * 65254eb Roll Dart SDK from e9a03fd98faa to 5acaa5f14b03 (1 revision) (flutter/engine#22810) * 3926b21 Roll Fuchsia Linux SDK from Bnaeivv07... to W14Qninrb... (flutter/engine#22817) * 5eb505f Roll Fuchsia Mac SDK from 7O11wjLVX... to Z_-ciOYM9... (flutter/engine#22820) * d85cb10 add trace kernel flag to allowlist (flutter/engine#22812) * 14cb066 [embedder] Compositor can specify that no backing stores be cached (flutter/engine#22780) * eb6eabc Reland "Introduce a delegate class for gpu metal rendering (#22611)" (flutter/engine#22777) * 644dd65 Temporarily reduce e2e test matrix to stop flaky web engine builds (flutter/engine#22824) * 105004d Stop using the List constructor. (flutter/engine#22793) * 34f49a1 Roll Dart SDK from 5acaa5f14b03 to cfaa7606cbf5 (2 revisions) (flutter/engine#22827) * 6c8342f Revert "Fix race condition in key event handling on Android (#22658)" (flutter/engine#22823) * 1c2a8f9 Roll Skia from c504ecda03b8 to 9443d58af292 (16 revisions) (flutter/engine#22828) * b63e911 Better handle image codec instantiation failure (flutter/engine#22809) * 1358fda Generate Maven metadata files for engine artifacts (flutter/engine#22685) * 079c669 Generate gen_snapshot_armv7 and gen_snapshot_arm64 (flutter/engine#22818) * fcbfa9f Split AOT Engine Runtime (flutter/engine#22624) * 24d289e Roll Fuchsia Linux SDK from W14Qninrb... to M_8svVndh... (flutter/engine#22842) * 78b567f Reland: "Fix race condition in key event handling on Android (#22658)" (flutter/engine#22834) * 7d32cea (MacOS) Add FlutterGLCompositor with support for rendering multiple layers (flutter/engine#22782) * a713174 Roll Skia from 9443d58af292 to c7112edbe0f4 (10 revisions) (flutter/engine#22839) * bee352c Roll Dart SDK from cfaa7606cbf5 to 97cfd05b3cb3 (2 revisions) (flutter/engine#22840) * e5f510f [web] Fix event transform between mousedown/up due to mouse move event (flutter/engine#22813) * 04b98dc Roll Fuchsia Mac SDK from Z_-ciOYM9... to DRN4P3zbe... (flutter/engine#22841) * 0e3b2cf Roll Skia from c7112edbe0f4 to d39aec0e40ec (17 revisions) (flutter/engine#22844) * e71c6f4 leaving only html tests (flutter/engine#22846) * 3773835 Make CkPicture resurrectable (flutter/engine#22807) * bd394a1 Roll Skia from d39aec0e40ec to 38921cafe1bb (7 revisions) (flutter/engine#22847) * 66f44c6 Roll Dart SDK from 97cfd05b3cb3 to a37a4d42e53d (4 revisions) (flutter/engine#22849) * bdadaad Add delayed event delivery for Linux. (flutter/engine#22577) * 48befc5 More rename from GPU thread to raster thread (flutter/engine#22819) * 9b1b7f6 Roll Skia from 38921cafe1bb to abcc1ecdfd0c (8 revisions) (flutter/engine#22851) * 14a6fd9 Fix NPE when platform plugin delegate is null (flutter/engine#22852)

Artur Rymarz added 2 commits October 8, 2018 12:58

add parameters to allow hiding icons and border of the Cupertino TabBar

2447980

add comments

f6c6295

googlebot added the cla: yes label Oct 8, 2018

Artur Rymarz added 3 commits October 8, 2018 13:22

fix analyzer issues

a001159

remove redundant whitespace

fe6b4d1

update AUTHORS

918c1a2

Artur Rymarz added 5 commits October 10, 2018 07:53

allow to customize border of Cupertino BottomTabBar

f646aa6

update test

a476cd1

whitespace analyzer warning

cf0dd40

refactor only-icons mode for bottom navigation bar

282e293

analyzer warning

158936f

xster reviewed Oct 10, 2018

View reviewed changes

artur-ios-dev added 2 commits October 10, 2018 22:03

adjust code to review suggestions

84c8116

analyzer warnings

418bf69

Merge branch 'master' into improvement/cupertino-bottom-tabbar

ab0b29c

xster approved these changes Oct 11, 2018

View reviewed changes

Artur Rymarz added 3 commits October 11, 2018 09:30

code adjustments after code review

9e42610

improve test for nullable titles in cupertino bottom bar

e7da126

single quotes for asset

0968da5

Artur Rymarz added 3 commits October 11, 2018 09:55

analyzer warnings

c8d099d

few more code improvement

0a24f9a

always_put_control_body_on_new_line analyzer warning

300b219

xster reviewed Oct 11, 2018

View reviewed changes

Artur Rymarz added 2 commits October 12, 2018 08:04

revert some auto-formats

f7ec15b

remove whitespace

50153aa

revert one more auto-format change

c9c5ba8

xster merged commit a3670a2 into flutter:master Oct 12, 2018

This was referenced Oct 16, 2018

Make text on BottomNavigationBarItem optional #17099

Closed

Make CupertinoTabBar and TabBar (and scaffolds) more customizable #15863

Closed

engine-flutter-autoroll mentioned this pull request Dec 4, 2020

Roll Engine from 20caf5496951 to 14a6fd97ca12 (67 revisions) #71699

Merged

engine-flutter-autoroll mentioned this pull request Dec 4, 2020

Roll Engine from 20caf5496951 to 6a5971dac761 (70 revisions) #71701

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 4, 2020

6625308 Revert "Roll buildroot and benchmark (flutter#22804)" (flutte…

6ba6721

…r/engine#22816)

cbracken pushed a commit to engine-flutter-autoroll/flutter that referenced this pull request Dec 4, 2020

6625308 Revert "Roll buildroot and benchmark (flutter#22804)" (flutte…

636bf88

…r/engine#22816)

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

Add parameters to allow hiding icons and border of the Cupertino TabBar #22804

Add parameters to allow hiding icons and border of the Cupertino TabBar #22804

Uh oh!

Conversation

artur-ios-dev commented Oct 8, 2018

Uh oh!

xster commented Oct 9, 2018

Uh oh!

artur-ios-dev commented Oct 9, 2018

Uh oh!

artur-ios-dev commented Oct 10, 2018

Uh oh!

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

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!

artur-ios-dev Oct 10, 2018 • 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!

artur-ios-dev commented Oct 10, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

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

Choose a reason for hiding this comment

Uh oh!

artur-ios-dev commented Oct 11, 2018

Uh oh!

xster left a comment

artur-ios-dev Oct 10, 2018 •

edited

Loading

artur-ios-dev commented Oct 10, 2018 •

edited

Loading

artur-ios-dev commented Oct 12, 2018 •

edited

Loading