Make DropdownButton's disabledHint and hint behavior consistent by shihaohong · Pull Request #42479 · flutter/flutter

shihaohong · 2019-10-10T23:45:08Z

Description

This PR brings consistency to DropdownButton's hint behavior in the following ways:

When DropdownButton.selectedItemBuilder = null, it now correctly wraps the hint with a Container with the correct constraints and alignment (To match the defaults of DropdownMenuItem).
When DropdownButton.selectedItemBuilder is defined, hint and disabledHint are no longer wrapped with the constrained Container, allowing for more flexibility in creating the hint and disabledHint widgets.

Related Issues

Fixes #42472
Fixes #43040

Tests

I added the following tests:

DropdownButton.selectedItemBuilder defined

size matches largest item in menu items
size matches largest item when hint is provided
size matches largest item when disabled hint is provided

General tests

tests to verify that for disabled dropdown buttons, hint is used when disabledHint is null

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

Does your PR require Flutter developers to manually update their apps to accommodate your change?

Yes, this is a breaking change (Please read Handling breaking changes). Replace this with a link to the e-mail where you asked for input on this proposed change.
No, this is not a breaking change.

shihaohong · 2019-10-14T15:42:04Z

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

+      }
+
+      if (widget.selectedItemBuilder == null) {
+        displayedHint = Container(


This Container mimics the constraints and alignments for a default menu item, similar to DropdownMenuItem, when selectedItemBuilder is not being used

It would probably be safest to either include a comment here and in DropdownMenuItem.build() indicating that the code must be kept in sync, or add a _DropdownMenuItemContainer widget and use it in both places. The widget approach might be preferable, since you can explain what's going on vis-a-vis selectedItemBuilder in a comment above the widget class.

Sounds good, I just added a _DropdownMenuItemContainer and had DropdownMenuItem extend it to keep the code in sync. PTAL

HansMuller · 2019-10-14T19:02:14Z

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

-      final Widget emplacedHint = _enabled
-        ? widget.hint
-        : DropdownMenuItem<Widget>(child: widget.disabledHint ?? widget.hint);
+      Widget displayedHint;


It doesn't really matter but this could be:

Widget displayedHint = _enabled ? widget.hint : widget.disabledHint ?? widget.hint;

Also: thank you for eliminating the name "emplacedHint" :-).

HansMuller · 2019-10-14T19:08:45Z

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

+      }
+
+      if (widget.selectedItemBuilder == null) {
+        displayedHint = Container(


It would probably be safest to either include a comment here and in DropdownMenuItem.build() indicating that the code must be kept in sync, or add a _DropdownMenuItemContainer widget and use it in both places. The widget approach might be preferable, since you can explain what's going on vis-a-vis selectedItemBuilder in a comment above the widget class.

HansMuller · 2019-10-14T19:13:05Z

packages/flutter/test/material/dropdown_test.dart

              );
            }).toList(),
+            selectedItemBuilder: selectedItemBuilder,
+            itemHeight: itemHeight,


Good: tests that don't specify itemHeight should work as before since kMinInteractiveDimension is the default.

HansMuller · 2019-10-14T19:15:51Z

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

    int hintIndex;
    if (widget.hint != null || (!_enabled && widget.disabledHint != null)) {
-      final Widget emplacedHint = _enabled
-        ? widget.hint


Previously, an app that specified a hint for an enabled DropdownButton, would not see that hint aligned and its minHeight would not be constrained (because we didn't wrap it). That seems like a pointless inconsistency, but we'll need to watch out for problems introduced by this.

Right -- this would be the breaking change part of this PR, I think it might still be worth sending an email out to Flutter developers to notify them of the correct fix if they run into problems here

HansMuller · 2019-10-14T19:19:54Z

packages/flutter/test/material/dropdown_test.dart

+        disabledHint: null,
+      );
+    }
+    // [hint] should display when [items] is null and [disabledHint] is not defined


According to https://api.flutter.dev/flutter/material/DropdownButton/hint.html, the hint is displayed if value is null.

There seems to be some pre-existing confusion about this. See https://api.flutter.dev/flutter/material/DropdownButton/value.html. Can you make the docs consistent about when the hint is displayed, and what exactly makes the button enabled/disabled?

I've updated the docs to be consistent with what exactly makes a button enabled/disabled.

HansMuller

LGTM.
This is a now a twofer!

HansMuller · 2019-10-21T23:30:43Z

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

+  /// The value of the currently selected [DropdownMenuItem].
+  ///
+  /// If [value] is null and [hint] is non-null, the [hint] widget is
+  /// displayed as a placeholder for the dropdown button.


... for the dropdown button's value.

HansMuller · 2019-10-21T23:31:11Z

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

+  /// A placeholder widget that is displayed by the dropdown button.
+  ///
+  /// If [value] is null, this widget is displayed as a placeholder for
+  /// the dropdown button. This widget is also displayed if the button


dropdown button => dropdown button's value

HansMuller · 2019-10-21T23:31:54Z

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

  /// displayed in grey and it will not respond to input. A disabled button
-  /// will display the [disabledHint] widget if it is non-null.
+  /// will display the [disabledHint] widget if it is non-null. If
+  /// [disabledHint] is also null but [hint] is non-null, [hint] will instead


will instead be displayed => will be displayed instead

…abled-hint

…ter#42479) * Fix DropdownButton disabledHint behavior * Fix hint behavior when selectedItemBuilder is null * Improve variable names, some formatting updates * Create _DropdownMenuItemContainer widget * Improve API docs to be consistent with hint/disabledHint actual behavior

shihaohong added 2 commits October 10, 2019 16:16

Fix DropdownButton disabledHint behavior

b44d26c

Fix hint behavior when selectedItemBuilder is null

48d124d

shihaohong added framework flutter/packages/flutter repository. See also f: labels. f: material design flutter/packages/flutter/material repository. labels Oct 10, 2019

shihaohong requested a review from HansMuller October 10, 2019 23:45

googlebot added the cla: yes label Oct 10, 2019

shihaohong changed the title ~~Make DropdownButton's disabledHint and hint behavior consistent~~ WIP: Make DropdownButton's disabledHint and hint behavior consistent Oct 10, 2019

shihaohong added work in progress; do not review and removed work in progress; do not review labels Oct 10, 2019

shihaohong changed the title ~~WIP: Make DropdownButton's disabledHint and hint behavior consistent~~ Make DropdownButton's disabledHint and hint behavior consistent Oct 10, 2019

Improve variable names, some formatting updates

442f523

shihaohong commented Oct 14, 2019

View reviewed changes

HansMuller reviewed Oct 14, 2019

View reviewed changes

shihaohong added 4 commits October 14, 2019 16:05

Create _DropdownMenuItemContainer widget

7cf1700

Improve API docs to be consistent with hint/disabledHint actual behavior

728e421

Code cleanup

e95d141

Add a newline to clean up code

e7c18c1

HansMuller approved these changes Oct 21, 2019

View reviewed changes

shihaohong added 2 commits October 22, 2019 09:01

Merge branch 'master' of github.com:flutter/flutter into dropdown-dis…

e725bd5

…abled-hint

Improve api doc grammar

c3e3531

shihaohong added the c: API break Backwards-incompatible API changes label Oct 22, 2019

shihaohong merged commit 734ddd3 into flutter:master Oct 28, 2019

shihaohong deleted the dropdown-disabled-hint branch October 28, 2019 18:00

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

Conversation

shihaohong commented Oct 10, 2019 • edited by HansMuller Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issues

Tests

Checklist

Breaking Change

Uh oh!

shihaohong Oct 14, 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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

shihaohong commented Oct 10, 2019 •

edited by HansMuller

Loading

shihaohong Oct 14, 2019 •

edited

Loading