Add .separated constructor to ReorderableListView
#168834
Conversation
github-actions
bot
added
framework
Fintasys
changed the title
.separated constructor to ReorderableListView
|
@victorsanni Thanks for the detailed review ! I tried to address all of your concerns, please have another look 🙇 |
There was a problem hiding this comment.
Thanks for responding! Can you also attach videos/screenshots to the PR description comparing before/after? This will make the PR easier to review.
The example and tests are also really nice. For now, can you attach the example code to the PR description, and add the example/example test in a subsequent PR? The leaner the PR, the easier it is to review.
Thanks once again.
| throw FlutterError('Every item of ReorderableListView must have a key.'); | ||
| // Separators built by widget.itemBuilder (when widget.separatorBuilder is non-null and index is odd) | ||
| // might not have keys or might have non-GlobalKeys. | ||
| // This assertion should only apply to actual reorderable items. |
There was a problem hiding this comment.
Added tests for these use cases 👍
| await drag.up(); | ||
| } | ||
|
|
||
| testWidgets('Reorder list item with separators', (WidgetTester tester) async { |
There was a problem hiding this comment.
This is testing the reorderable behavior? Is there a test like this in reorderable_list_test.dart?
There was a problem hiding this comment.
Yes it's testing the reorderable behavior, similar to the other constructors.
There is no behavior test in reorderable_list_test.dart which is also aligned with the other constructors.
There was a problem hiding this comment.
Example tests should only confirm that visually, all intended widgets are displayed. All behavior tests should be in the actual widget test file. There may be places in the codebase where that has slipped through the cracks but that is the original purpose.
There was a problem hiding this comment.
Okay I moved the test and created simple visual regression test for the example 👍
| @@ -0,0 +1,65 @@ | |||
| // Copyright 2014 The Flutter Authors. All rights reserved. | |||
There was a problem hiding this comment.
Does this example generate the video in the PR description? In the video, the way the separators move with the tiles looks off. For example, a separator appears at the top if the first tile is reordered. Also, the separators appear above the tiles while reordering. Can the example be updated to fix these and others?
There was a problem hiding this comment.
I agree the behavior looks a bit off with the default Divider. In my own app the Divider appearance is different and doesn't look & feel off.
But in order to improve this behavior I would need to make changes to the underlying SliverReorderableList and e.g. expose a callback that allows the SliverReorderableList to understand between draggable and non-draggable widgets. That would also reintroduce childCount and the helper function that we just removed.
I'm worried that this goes beyond of the current scope of this PR and might be better addressed in a separate PR?
There was a problem hiding this comment.
is it possible to expand the example to use another ReorderableListView property to achieve this? For example, onReorder or proxyDecorator.
There was a problem hiding this comment.
No, unfortunately I don't think so.
-
onReorder: This callback is invoked after the item is dropped and is used to update the data model. It doesn't affect the visual behavior during the drag. -
proxyDecorator: This callback is to customize the appearance of the item being dragged (the proxy). It does not control the behavior or rendering of the other items or the separators in the list.
This PR introduces a
.separatedconstructor to theReorderableListViewwidget. The.separatedconstructor allows developers to define custom separators between the list items without the need to manually modify their list or create custom widgets. This API enhancement improves the overall usability and flexibility ofReorderableListView.Key Changes:
.separatedconstructor toReorderableListView..separatedfunctionality.This new feature addresses the requests in issue #76706.
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.
Video
separated_example.mp4