Added single open panel functionality #19624

jslavitz · 2018-07-20T21:16:09Z

fixes #19240

jonahwilliams · 2018-07-20T21:20:36Z

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

  /// The duration of the expansion animation.
  final Duration animationDuration;

+  //Whether multiple panels can be open simultaneously


Nit: space between // and text

jonahwilliams · 2018-07-20T21:20:49Z

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

+    if (!widget._allowMultiplePanelsOpen) {
+      for (int i = 0; i < widget.children.length; i += 1) {
+        assert(widget.children[i] is ExpansionPanelRadio,
+        'All children of ExpansionPanel.radio need to be of type ExpansionPanelRadio');


indent with 2 spaces

jonahwilliams · 2018-07-20T21:20:58Z

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

+      }
+
+      assert(_debugCheckExpandLimit(),
+      'This expansion panel widget initialized with more than one panel open');


jonahwilliams · 2018-07-20T21:22:20Z

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

+  @override
+  void initState() {
+    super.initState();
+    if (!widget._allowMultiplePanelsOpen) {


Since you are always using the negation of this field, consider changing it to _preventMultiplePanelsOpen or something similar

jonahwilliams · 2018-07-20T21:23:10Z

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

+      }
+      _openPanels[pressedChild.value] = !isExpanded;
+    }
+    setState((){});


You only want to setState if you aren't allowing multiple panels

jonahwilliams · 2018-07-20T21:23:23Z

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

+
+  void _handlePressed(bool isExpanded, int index) {
+    if (widget.expansionCallback != null)
+      widget.expansionCallback(index, isExpanded);


Does this callback get invoked twice in radio mode?

You mean by the rest of the code below? I don't think so, only possibly one other panel if it was open before.

jonahwilliams · 2018-07-20T21:23:37Z

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

                  context,
-                  children[index].isExpanded,
+                  widget._allowMultiplePanelsOpen ? widget.children[index].isExpanded :
+                  _openPanels[_widgetChild.value],


Indent with 2 lines

jonahwilliams · 2018-07-20T21:24:04Z

packages/flutter/test/material/expansion_panel_test.dart

+
+  testWidgets('Single Panel Open Test',  (WidgetTester tester) async {
+
+    List<ExpansionPanel> _demoItems;


Initialize variables where they are declared

jonahwilliams · 2018-07-23T17:37:53Z

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

  }) : assert(headerBuilder != null),
-       assert(body != null),
-       assert(isExpanded != null);
+        assert(body != null),


Nit: keep asserts lined up

jonahwilliams · 2018-07-23T20:43:24Z

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

+      bool panelExpanded = false;
+      for (int i = 0; i < widget.children.length; i += 1) {
+        assert(widget.children[i] is ExpansionPanelRadio,
+        'All children of ExpansionPanel.radio need to be of type ExpansionPanelRadio');


Indent by 2 spaces

jonahwilliams · 2018-07-23T20:43:32Z

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

+        final int childKey = child.value;
+        final bool panelWillBeExpanded = child.initializesExpanded || (childKey == _currentOpenPanel?.value);
+        assert(!panelExpanded && panelWillBeExpanded || panelExpanded && !panelWillBeExpanded,
+               'Trying to initialize radio panel list with more than two open panels!');


Indent by 2 spaces

jonahwilliams · 2018-07-23T20:45:38Z

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

+  void initState() {
+    super.initState();
+    if (widget._allowOnlyOnePanelOpen) {
+      for (int i = 0; i < widget.children.length; i += 1) {


Since you don't need the index i for anything, use for (ExpansionPanelRadio child in widget.children), here and elsewhere. I don't think you need to assert in initState unless you are dealing with the radio -> non radio or reverse

jonahwilliams · 2018-07-23T20:45:56Z

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

+  @override
+  void didUpdateWidget(ExpansionPanelList oldWidget) {
+    super.didUpdateWidget(oldWidget);
+


What happens if I change from radio to non-radio or likewise?

jonahwilliams · 2018-07-23T20:47:12Z

packages/flutter/test/material/expansion_panel_test.dart

    expect(tester.getRect(find.byType(AnimatedSize).at(2)), new Rect.fromLTWH(0.0, 56.0 + 1.0 + 56.0 + 16.0 + 16.0 + 48.0 + 16.0, 800.0, 100.0));
  });
+
+  testWidgets('Single Panel Open Test',  (WidgetTester tester) async {


Make sure you test adding/removing panels in radio mode (especially the open panel), and switch from radio to non radio

jonahwilliams · 2018-07-24T21:10:52Z

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

-  }) : assert(headerBuilder != null),
-       assert(body != null),
-       assert(isExpanded != null);
+  }) :  assert(headerBuilder != null),


Remove the extra spaces, here and below in the initializer list.

jonahwilliams · 2018-07-24T21:11:51Z

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

+
+/// An expansion panel that allows for radio-like functionality.
+///
+/// This type of panel will automatically handle closing and opening itself,


Try and keep these doc comments to 80 columns

jonahwilliams · 2018-07-24T21:14:26Z

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

+/// An expansion panel that allows for radio-like functionality.
+///
+/// This type of panel will automatically handle closing and opening itself,
+/// set the [initializesExpanded] value to true on a panel if you want it to begin


I'm not sure about placing initializesExpanded on the RadioPanel. Try adding another named argument to the ExpansionPanelList.radio constructor which takes a panel or value of a panel to open "only the first time it is created". Then don't both updating in the future, it doesn't make sense to try and keep all of the initializesExpanded and state changed by user interaction in sync.

jonahwilliams · 2018-07-24T21:14:44Z

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

+    @required this.value,
+    @required ExpansionPanelHeaderBuilder headerBuilder,
+    @required Widget body,
+  }) :  assert(initializesExpanded != null),


Same as above, 1 space here.

jonahwilliams · 2018-07-24T21:15:09Z

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

+  /// Whether this panel initializes expanded or not.
+  final bool initializesExpanded;
+
+  /// Identifier that corresponds to this specific object.


"The value that uniquely identifies a radio panel"

Also this should be Object like I said, otherwise I can't use a String key

jonahwilliams · 2018-07-24T21:16:58Z

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

+          _currentOpenPanel = child;
+      }
+    }
+    else if(oldWidget._allowOnlyOnePanelOpen) {


Move this to the previous line so that it directly follows the previous closing }.

jonahwilliams · 2018-07-24T21:18:09Z

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

+    }
+    else if(oldWidget._allowOnlyOnePanelOpen) {
+      _currentOpenPanel = null;
+      setState((){});


You don't/shouldn't setState in didUpdateWidget, because your state is already dirty. Might have been a miscommunication - this is only required in _handlePressed

jonahwilliams · 2018-07-24T21:18:47Z

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

      if (_isChildExpanded(index) && index != 0 && !_isChildExpanded(index - 1))
        items.add(new MaterialGap(key: new _SaltedKey<BuildContext, int>(context, index * 2 - 1)));

+      final ExpansionPanelRadio _widgetChild = widget._allowOnlyOnePanelOpen ? widget.children[index] : null;


Don't make locals private, rename to "radioPanel"

jonahwilliams · 2018-07-24T21:19:49Z

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

+                child: widget.children[index].headerBuilder(
                  context,
-                  children[index].isExpanded,
+                  widget._allowOnlyOnePanelOpen ? _widgetChild.value == _currentOpenPanel?.value:


use the _isChildExpanded callback

jonahwilliams · 2018-07-24T21:30:58Z

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

+  /// [ExpansionPanelRadio].
+  const ExpansionPanelList.radio({
+    Key key,
+    this.children = const <ExpansionPanelRadio>[],


Something I missed earlier - this isn't a type annotation for children, it is just the type for the default value. You need to use an explicitly typed param with an initializer.

({ List<ExpansionPanelRadio> children = .., }) : children = children,

gspencergoog · 2018-07-25T17:32:26Z

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

+/// A unique identifier [value] must be assigned to each panel.
+class ExpansionPanelRadio extends ExpansionPanel {
+
+  /// A subclass of expansion panel that allows for radio functionality.


Just say "An expansion panel" like you had above. Don't put the "subclass of" part in because if that changes, it's easy for the docs to get missed and be out of date.

gspencergoog · 2018-07-25T17:33:41Z

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

+
+  /// A subclass of expansion panel that allows for radio functionality.
+  ///
+  /// A unique identifier [value] must be passed into the constructor. The


Take out the word "identifier": it reads better as "A unique [value]".

Also, explain why it must be unique: say something like "The [value] is used to indicate which panel is currently selected."

gspencergoog · 2018-07-25T17:36:33Z

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

+  }) : assert(value != null),
+       super(body: body, headerBuilder: headerBuilder);
+
+  /// The value that uniquely identifies a radio panel.


Add why it exists: "so that the currently selected radio panel may be identified."

gspencergoog · 2018-07-25T17:39:15Z

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

+  final bool _allowOnlyOnePanelOpen;
+
+  /// The value of the panel that initially begins open. (This value is
+  /// only used when initializing with the radio constructor.)


radio -> [ExpansionPanelList.radio]

gspencergoog · 2018-07-25T17:48:38Z

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

              child: new ConstrainedBox(
                constraints: const BoxConstraints(minHeight: _kPanelHeaderCollapsedHeight),
-                child: children[index].headerBuilder(
+                child: widget.children[index].headerBuilder(


Since it's used more than once, you can add:

final ExpansionPanelRadio child = widget.children[index];

above, and use child.headerBuilder( here, and below.

gspencergoog · 2018-07-25T17:50:59Z

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

+  /// This widget allows for at most one panel in the list to be open.
+  /// The expansion panel callback is triggered when an expansion panel
+  /// expand/collapse button is pushed. The [children] and [animationDuration]
+  /// arguments must not be null. The [children] objects also must of type


"objects also must of type" --> "objects must be instances of"

gspencergoog · 2018-07-25T19:51:34Z

Just commiting two files now

09ea4e7

googlebot added the cla: yes label Jul 20, 2018

Fixed one tab

55cbac8

jonahwilliams suggested changes Jul 20, 2018

View reviewed changes

jslavitz added 2 commits July 20, 2018 15:12

Fixed latest changes

5de9df3

Changed the data structure from a map to a single radio panel object

c2a81d7

jonahwilliams suggested changes Jul 23, 2018

View reviewed changes

jslavitz added 3 commits July 24, 2018 11:05

A few more changes

6f57ac7

Fixed change from expansion radio to regular list

1df7b5b

Fixed change from expansion radio to regular list2

1fd0380

jonahwilliams suggested changes Jul 24, 2018

View reviewed changes

jslavitz and others added 2 commits July 24, 2018 15:45

Changed the radio constructor

bc08262

Last fixes

96c8fe8

googlebot added cla: no and removed cla: yes labels Jul 25, 2018

Final commit

579aa36

jonahwilliams approved these changes Jul 25, 2018

View reviewed changes

jonahwilliams requested a review from gspencergoog July 25, 2018 17:29

gspencergoog reviewed Jul 25, 2018

View reviewed changes

Actual final commit

ed57c1f

gspencergoog approved these changes Jul 25, 2018

View reviewed changes

Last change

3c3b030

jslavitz added cla: yes and removed cla: no labels Jul 25, 2018

flutter deleted a comment from googlebot Jul 25, 2018

jslavitz merged commit 28dc007 into flutter:master Jul 25, 2018

jslavitz deleted the expansionPanel3 branch December 3, 2018 22:29

shihaohong mentioned this pull request Apr 17, 2019

Exception on ExpansionPanel toggle due to Duplicate Global Keys #13780

Closed

shihaohong mentioned this pull request Apr 17, 2019

Fix ExpansionPanelList Duplicate Global Keys Exception #31228

Merged

9 tasks

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


		testWidgets('Single Panel Open Test', (WidgetTester tester) async {

		List<ExpansionPanel> _demoItems;

Added single open panel functionality #19624

Added single open panel functionality #19624

Uh oh!

Conversation

jslavitz commented Jul 20, 2018

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!

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!

gspencergoog commented Jul 25, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants