Reorderable list widget and Material demo by DaveShuckerow · Pull Request #18374 · flutter/flutter

DaveShuckerow · 2018-06-11T21:09:32Z

This introduces a widget, ReorderableList, which wraps its children inside of a LongPressDraggable, and constrains the drag to a single axis.

It also uses Scrollable.ensureVisible to allow the user to scroll through the reorder operation.

…shuffle-list

…fle-list

…shuffle-list

…fle-list

…shuffle-list

DaveShuckerow · 2018-07-11T21:03:21Z

I met up with Jonah about the accessibility tests we ran over the widget.

Selected list item loses focus when dragging (Accessibility: Feedback widget of Draggables is not part of the Semantics tree #18428):We've resolved this.
List item does not call out long press action (LongPressDraggable doesn't have accessibility semantics for its long press action in iOS #18585): This isn't expected behavior, so it's not relevant.
Support for local context menu to move items up/down in list (Accessibility support for custom actions #18432): Jonah has a PR for Flutter Engine that adds this support.

Mulligan also reported that they needed support for a non-reorderable List Header. I've added that into this list.

Landing the fix to #18432 is currently the last known blocker to landing this widget.

DaveShuckerow · 2018-07-19T20:11:07Z

@HansMuller
@jonahwilliams
@alanrussian

The only remaining blocker I'm aware of is to add support for custom accessibility actions (#18882).

Please let me know if there is anything else you believe this PR needs.

alanrussian · 2018-07-19T21:37:29Z

@DaveShuckerow there's one issue we have. I sent you an email.

…fle-list

HansMuller

Will chat with you about this.

HansMuller · 2018-07-20T00:01:12Z

examples/flutter_gallery/lib/demo/material/reorderable_list_demo.dart

+    setState(() {
+      _itemType = type;
+    });
+    _bottomSheet?.setState(() { });


These two lines are confusing. If _bottomSheet can be null, then don't both references need ?.. And why rebuild the bottom sheet if we're going to close it? A comment would be helpful here.

Correct. I've updated and added comments.

HansMuller · 2018-07-20T00:06:36Z

examples/flutter_gallery/lib/demo/material/reorderable_list_demo.dart

+  }
+
+  void _showConfigurationSheet() {
+    final PersistentBottomSheetController<Null> bottomSheet = scaffoldKey.currentState.showBottomSheet((BuildContext bottomSheetContext) {


Why not just set _bottomSheet here?

HansMuller · 2018-07-20T00:06:39Z

examples/flutter_gallery/lib/demo/material/reorderable_list_demo.dart

+      );
+    });
+
+    setState(() {


I've applied your suggested change.
The setState() is still here. What's it for?

HansMuller · 2018-07-20T00:07:19Z

examples/flutter_gallery/lib/demo/material/reorderable_list_demo.dart

+    // Garbage collect the bottom sheet when it closes.
+    bottomSheet.closed.whenComplete(() {
+      if (mounted) {
+        setState(() {


Why is a _setState needed here?

There was a comment I had left that's hidden by the commit history now.

I applied the change, but not setting state led to strange behavior: the menu in the corner doesn't update to reflect that you can open the bottom sheet again.

HansMuller · 2018-07-20T00:10:31Z

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

+      assert(onReorder != null),
+      assert(children != null),
+      assert(
+        children.every((Widget w) => w.key != null), 


HansMuller · 2018-07-20T00:29:38Z

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

+  }
+
+  @override
+  void dispose() {


I don't think we want this widget at all, but this override clearly isn't needed.

We discussed this offline, and agreed we can compute the size of the dragging widget when it's first picked.

Now doing this with the global keys I added for semantics compatibility.

HansMuller · 2018-07-25T22:12:44Z

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

-  
+
  @override
  State<StatefulWidget> createState() {


I'd mentioned this before. CreateState methods like this are boilerplate and the convention for their format is:

_ReorderableListViewState createState() => new _ReorderableListViewState();

HansMuller · 2018-07-25T22:40:24Z

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

@@ -312,6 +312,13 @@ class _ReorderableListContentState extends State<_ReorderableListContent> with T
  // Handles up the logic for dragging and reordering items in the list.
  Widget _wrap(Widget toWrap, int index, BoxConstraints constraints) {


If I provide a list of three stateful children with keys A, B, and C then if I rebuild the ReorderableList with the same children ordered C, B , A then I'd expect that the state of C, B, and A will have been preserved. Because they appear to be siblings, even though they're not actually siblings. In other words, I would not expect any of the the A, B, C widgets' state to be disposed.

Since each GlobalKey includes the child's index, I don't think child state will be preserved if the ReorderableList is rebuilt with the same children, but in a different order.

We should probably have a test that checked this.

HansMuller · 2018-07-26T22:09:09Z

LGTM and hooray!

…fle-list

This reverts commit 6f029f5.

This reverts commit dc7d648.

knopp · 2018-07-30T23:34:48Z

The reordering threshold is still wrong. It is different from both android and material (at least what I tried in Google Chrome settings on Android).

When reordering up the behavior is same as on iOS (threshold being top border touching middle of item above), different from Material (where the threshold is top border touching the top border of item above).

When reordering down the behavior is different from both of them. on iOS the bottom border of dragged item needs to touch middle of item below, in Material the bottom border of dragged item needs to touch bottom of item below, in this implementation the top border of dragged item needs to touch middle of item below, which means it needs to be dragged way too far. Any plans addressing this?

knopp · 2018-07-30T23:35:52Z

Also, the moment shadows appears the items below are shifted down a bit (probably due to the shadow). That doesn't feel right.

DaveShuckerow · 2018-08-07T20:05:37Z

Hi Matej, thanks for chiming in! The discerning eye of our community members is extremely valuable as we plan future work.

If you look at the issue that this PR fixes, you can see we do have other open issues related to this widget. If you think something is off in the implementation that those issues don't discuss, please feel free to file an issue of your own so that we don't lose track of it.

Dave Shuckerow added 30 commits April 20, 2018 14:07

Try out shuffling a list around

579f334

Merge branch 'shuffle-list' of github.com:DaveShuckerow/flutter into …

cbafa47

…shuffle-list

Merge branch 'master' of https://github.com/flutter/flutter into shuf…

a272686

…fle-list

Merge branch 'master' of https://github.com/flutter/flutter into shuf…

7bfa95a

…fle-list

Limit the axis of the drag

528c64d

Add an empty space at the bottom to drop tiles into

fbab957

Add a tappable target for demo purposes

79ca233

Merge branch 'master' of https://github.com/flutter/flutter into shuf…

b30c372

…fle-list

Merge branch 'shuffle-list' of github.com:DaveShuckerow/flutter into …

608b3d1

…shuffle-list

Make the list demo use the new drag

0dcc30e

Start using a higher-level widget for this change.

0a85050

Working reorder

48a5daa

Use a scroll controller to get finer-grained control over the scrolling

2629c5a

Remove unused code

17e2e0f

Merge branch 'master' of github.com:flutter/flutter into shuffle-list

3b824e1

Support reorder to the very bottom

414c6c0

Make scrolling work reasonably

7dc8a07

It all works

ef3e81c

Make scrolling better

61457ab

Make swaps at the end of the list smoother

37c515b

Remove debug statements

4cd4141

Merge branch 'master' of https://github.com/flutter/flutter into shuf…

b84fa50

…fle-list

Merge branch 'shuffle-list' of github.com:DaveShuckerow/flutter into …

f6134a5

…shuffle-list

Polish

e6938f0

Remove debug statements

9e734d8

Move the new list into a new file

1a31441

Move the new list into a new file

9c10be2

Comments

75f0383

Set up the reorderable list in a separate file, start docs

ed8e5fa

Add some tests for reorderable list.

d2c3507

Pull out some brackets

f285fe4

Remove unnecessary quiver import

79c4792

knopp mentioned this pull request Jul 12, 2018

Feature Request: Built-in List Drag/Drop Reordering #16763

Closed

zoechi mentioned this pull request Jul 19, 2018

ListView Drag&Drop Example #13651

Closed

Add a feedback offset to the reorderable list

a5ad47f

Merge branch 'master' of https://github.com/flutter/flutter into shuf…

2c1f693

…fle-list

HansMuller reviewed Jul 20, 2018

View reviewed changes

Dave Shuckerow added 6 commits July 20, 2018 11:08

Remove the feedback wrapper and get height from the global key instead

f488090

Remove the feedbackOffset.

75282fe

Comments on demo code.

ec7859c

Respond to comments

0e0247c

Remove whitespace

69df67c

Remove whitespace

d62755f

HansMuller reviewed Jul 25, 2018

View reviewed changes

Add a test for reordering the list and its children.

31b7501

Dave Shuckerow added 6 commits July 27, 2018 10:26

Merge branch 'master' of https://github.com/flutter/flutter into shuf…

ebb105c

…fle-list

Remove const in some places that the analyzer complained

dc7d648

Remove const in some places that the analyzer complained

6f029f5

Revert "Remove const in some places that the analyzer complained"

5b161bd

This reverts commit 6f029f5.

Revert "Remove const in some places that the analyzer complained"

a06a12d

This reverts commit dc7d648.

Remove whitespace

5b7da79

DaveShuckerow merged commit f844fad into flutter:master Jul 30, 2018

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

		@@ -312,6 +312,13 @@ class _ReorderableListContentState extends State<_ReorderableListContent> with T
		// Handles up the logic for dragging and reordering items in the list.
		Widget _wrap(Widget toWrap, int index, BoxConstraints constraints) {

Conversation

DaveShuckerow commented Jun 11, 2018

Uh oh!

DaveShuckerow commented Jul 11, 2018

Uh oh!

DaveShuckerow commented Jul 19, 2018

Uh oh!

alanrussian commented Jul 19, 2018

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!

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!

HansMuller commented Jul 26, 2018

Uh oh!

knopp commented Jul 30, 2018

Uh oh!

knopp commented Jul 30, 2018

Uh oh!

DaveShuckerow commented Aug 7, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants