[Material] Fix BottomNavTheme.showSelectedLabels bug by johnsonmh · Pull Request #67342 · flutter/flutter

johnsonmh · 2020-10-05T20:17:42Z

Description

This PR fixes a bug with the BottomNavigationBarThemeData.showSelectedLabels param. Because BottomNavigationBar.showSelectedLables was being set to true, in the constructor, that meant it was not null by default, as the rest of the implementation expected.

Also cleaned up some of the NNBD logic.

Related Issues

Fixes #66738
Fixes #67185

Tests

I added the following tests:

Test that BottomNavTheme.showSelectedLabels: false and BottomNavTheme.showUnselectedLabels: false works when no widget params provided.
Test that BottomNavTheme.showSelectedLabels: false and BottomNavTheme.showUnselectedLabels: true works when no widget params provided.
Test that BottomNavTheme.showSelectedLabels: true and BottomNavTheme.showUnselectedLabels: false works when no widget params provided.

Checklist

Before you create this PR, confirm that it meets all requirements listed below by checking the relevant checkboxes ([x]). This will ensure a smooth and quick review process.

I read the Contributor Guide and followed the process outlined there for submitting PRs.
I signed the CLA.
I read and followed the Flutter Style Guide, including Features we expect every widget to implement.
I read the Tree Hygiene wiki page, which explains my responsibilities.
I updated/added relevant documentation (doc comments with ///).
All existing and new tests are passing.
The analyzer (flutter analyze --flutter-repo) does not report any problems on my PR.
I am willing to follow-up on review comments in a timely manner.

Breaking Change

Did any tests fail when you ran them? Please read Handling breaking changes.

No, no existing tests failed, so this is not a breaking change.
Yes, this is a breaking change. If not, delete the remainder of this section.
- I wrote a design doc: https://flutter.dev/go/template Replace this with a link to your design doc's short link
- I got input from the developer relations team, specifically from: Replace with the names of who gave advice
- I wrote a migration guide: https://flutter.dev/go/breaking-changes-template Replace this with a link to a pull request that adds the migration guide to https://flutter.dev/docs/release/breaking-changes

…vBools

rami-a · 2020-10-05T20:35:02Z

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

        selected: i == widget.currentIndex,
-        showSelectedLabels: widget.showSelectedLabels ?? bottomTheme.showSelectedLabels,
+        showSelectedLabels: widget.showSelectedLabels ?? bottomTheme.showSelectedLabels ?? true,
        showUnselectedLabels: widget.showUnselectedLabels ?? bottomTheme.showUnselectedLabels ?? _defaultShowUnselected,


Should _defaultShowUnselected just be replaced with the value itself to match the line above?

No, because _defaultShowUnselected is a getter that depends on the BottomNavigationBar.type

rami-a · 2020-10-05T20:35:59Z

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

+              selectedIconTheme: selectedIconTheme,
+              unselectedIconTheme: unselectedIconTheme,
            ),
            _Label(
              colorTween: colorTween,
              animation: animation,
              item: item,
-              selectedLabelStyle: selectedLabelStyle ?? bottomTheme.selectedLabelStyle,
-              unselectedLabelStyle: unselectedLabelStyle ?? bottomTheme.unselectedLabelStyle,
-              showSelectedLabels: showSelectedLabels ?? bottomTheme.showUnselectedLabels,
-              showUnselectedLabels: showUnselectedLabels ?? bottomTheme.showUnselectedLabels,
+              selectedLabelStyle: selectedLabelStyle,
+              unselectedLabelStyle: unselectedLabelStyle,
+              showSelectedLabels: showSelectedLabels,
+              showUnselectedLabels: showUnselectedLabels,


whats the reason for this change?

The null coalescing here is redundant, because the values passed to _BottomNavigationTile have already done the null theme checks. This will also make the NNBD migration more clear.

#67078

…vBools

HansMuller · 2020-10-06T16:16:07Z

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

-  /// The [showSelectedLabels] argument must be non-null.
-  ///
-  /// The [showUnselectedLabels] argument defaults to `true` if [type] is
+  /// The [showSelectedLabels] argument defaults to `true`. The


This isn't strictly true (it defaults to null) and it doesn't cover what's actually happening: widget.showSelectedLabels ?? bottomTheme.showSelectedLabels ?? true.

Updated comment

HansMuller · 2020-10-06T16:17:00Z

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

-  ///
-  /// The [showUnselectedLabels] argument defaults to `true` if [type] is
+  /// The [showSelectedLabels] argument defaults to `true`. The
+  /// [showUnselectedLabels] argument defaults to `true` if [type] is


Similar comment to the previous one.

Updated comment

HansMuller · 2020-10-06T16:21:34Z

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

-    final double unselectedIconSize = unselectedIconTheme?.size
-      ?? bottomTheme.unselectedIconTheme?.size
-      ?? iconSize;
+    final double selectedIconSize = selectedIconTheme?.size ?? iconSize;


Why are we now ignoring bottomTheme.selectedIconTheme?.size? Here and on the next line.

We discussed this offline, but to summarize, the whole BottomNavigationBarThemeData.selectedIconTheme is ignored if BottomNavigationBar.selectedIconTheme is present. I updated the documentation on BottomNavigationBarThemeData to make that more clear.

HansMuller

LGTM

HansMuller · 2020-10-07T15:39:40Z

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

-  /// The [showSelectedLabels] argument must be non-null.
+  /// If [showSelectedLabels] is `null`, [BottomNavigationBarThemeData.showSelectedLabels]
+  /// is used. If [BottomNavigationBarThemeData.showSelectedLabels]  is null,
+  /// then [showSelectedLabels] defaults to `true`.argument defaults to `true`.


typo here - remove trailing argument defaults to true`.

Thanks for spotting this

HansMuller · 2020-10-07T15:40:11Z

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

-  /// The [showUnselectedLabels] argument defaults to `true` if [type] is
-  /// [BottomNavigationBarType.fixed] and `false` if [type] is
+  /// If [showUnselectedLabels] is `null`, [BottomNavigationBarThemeData.showUnselectedLabels]
+  /// is used. If [BottomNavigationBarThemeData.showSelectedLabels]  is null,


extra space before "is null"

pedromassangocode · 2020-10-22T06:59:04Z

Updating PR to close the related issues automatically.

…vBools

johnsonmh added 3 commits October 5, 2020 15:45

update docs

b4b6faf

Merge branch 'master' of github.com:flutter/flutter into fix-bottomNa…

ef78da3

…vBools

fix null param coalescing logic, add tests

0e2fcb9

flutter-dashboard bot added f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels Oct 5, 2020

googlebot added the cla: yes label Oct 5, 2020

johnsonmh requested review from HansMuller, clocksmith, rami-a and willlarche October 5, 2020 20:17

johnsonmh mentioned this pull request Oct 5, 2020

migrate some material files to nullsafety #67078

Merged

13 tasks

clocksmith approved these changes Oct 5, 2020

View reviewed changes

rami-a approved these changes Oct 5, 2020

View reviewed changes

johnsonmh added 3 commits October 5, 2020 16:40

remove unneeded theme

de9a50f

remove trailing space

0115232

Merge branch 'master' of github.com:flutter/flutter into fix-bottomNa…

2501a7a

…vBools

HansMuller reviewed Oct 6, 2020

View reviewed changes

johnsonmh added 2 commits October 6, 2020 16:21

update docs

a2413fb

update docs

6a5bfb6

HansMuller approved these changes Oct 7, 2020

View reviewed changes

xu-baolin mentioned this pull request Oct 10, 2020

Fix BottomNavigationBar can not apply some theme property #66771

Closed

johnsonmh added 2 commits October 22, 2020 14:24

docs typos

2407215

Merge branch 'master' of github.com:flutter/flutter into fix-bottomNa…

e36a0bd

…vBools

johnsonmh added the waiting for tree to go green label Oct 26, 2020

fluttergithubbot merged commit 70aa122 into flutter:master Oct 26, 2020

dayveedaniel mentioned this pull request Aug 13, 2022

TabBar labelStyle.color and unselectedLabelStyle.color does not take effect #109484

Closed

Conversation

johnsonmh commented Oct 5, 2020 • edited by pedromassangocode Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Tests

Checklist

Breaking Change

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!

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

pedromassangocode commented Oct 22, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

johnsonmh commented Oct 5, 2020 •

edited by pedromassangocode

Loading