Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ReorderableListView] remove extra margin added after picking up the item #65080

Merged
merged 4 commits into from
Sep 16, 2020

Conversation

nero-angela
Copy link
Contributor

@nero-angela nero-angela commented Sep 2, 2020

Description

As you can see under the gif when picking up an item in ReorderableListView, the others below the item move slightly downward. this is because the extra margin is added to the selected item after lifting. So I removed the extra margin and modified the related test.

before after
before after

Golden test image

reorderable_list_test.vertical.drop_area.png reorderable_list_test.horizontal.drop_area.png
reorderable_list_test vertical drop_area reorderable_list_test horizontal drop_area

Related Issues

Fixes #58411

Tests

I modified the following tests:

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.

@flutter-dashboard flutter-dashboard bot added f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels Sep 2, 2020
Copy link
Contributor

@dkwingsmt dkwingsmt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good change, with small change requests.

Also it'd be better if you can add a golden test to make sure it looks exactly how we want after picking up the item. If you're up for the challenge, refer to https://github.com/flutter/flutter/wiki/Writing-a-golden-file-test-for-package:flutter or search for matchesGoldenFile in existing test files.

@@ -207,7 +207,7 @@ class _ReorderableListContentState extends State<_ReorderableListContent> with T
static const double _defaultDropAreaExtent = 100.0;

// The additional margin to place around a computed drop area.
static const double _dropAreaMargin = 8.0;
static const double _dropAreaMargin = 0.0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This variable should no longer be necessary. Remove to simplify code.

// The list view pads the drop area by 8dp.
const double kDraggingListHeight = 300.0;
// The list view pads the drop area by 0dp.
const double kDraggingListHeight = 292.0;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Combine kNonDraggingListHeight and kDraggingListHeight to simplify code.

@nero-angela
Copy link
Contributor Author

nero-angela commented Sep 11, 2020

Thank you @dkwingsmt ! I've simplified the unnecessary code and added golden test. The images are in the description. :)
This is my first time making golden file changes, so I'm not sure if I did the golden test correctly. 🤔
Could you please take a look?

@dkwingsmt
Copy link
Contributor

The Linux analyzer seems unhappy. Can you resolve the lint errors?

@nero-angela
Copy link
Contributor Author

nero-angela commented Sep 12, 2020

sorry for missing declaring const. @dkwingsmt I've resolved the lint error.

Copy link
Contributor

@dkwingsmt dkwingsmt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@dkwingsmt
Copy link
Contributor

Theoretically the bot should comment that a golden test has been modified... I'll check it out next week.

@nero-angela
Copy link
Contributor Author

nero-angela commented Sep 12, 2020

Okay. Thank you for reviewing. Please let me know if there is anything I need to do.
Have a nice weekend :)

@justinmc
Copy link
Contributor

@dkwingsmt @nero-angela The golden tests added in this PR failed when I tried to revert a TextField layout PR from just before this: #66027

Can you guys think of any reason why that would happen? These new tests have nothing to do with TextField or InputDecoration layout, right?

@justinmc
Copy link
Contributor

Actually it seems to be happening on many PRs:
#61981
#65884

@Piinks
Copy link
Contributor

Piinks commented Sep 17, 2020

These images don't appear to have arrived on the gold dashboard after landing: https://flutter-gold.skia.org/
There is nothing under by blame, and I can't find reference to the tests. Checking the logs now on master.
fyi @kjlubick

@Piinks
Copy link
Contributor

Piinks commented Sep 17, 2020

Looks like our luci recipes have been changed and disabled gold again. Will file an infra ticket.

@Piinks
Copy link
Contributor

Piinks commented Sep 17, 2020

#64969 for tracking

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ReorderableListView listitems jump down on dragging
6 participants