Skip to content

Add DragTarget callback onAcceptDetails, plus helper class DragTarget… #55257

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.

Sign up for GitHub

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

Jump to bottom
Merged
goderbauer merged 3 commits into from
May 1, 2020
Merged

Add DragTarget callback onAcceptDetails, plus helper class DragTarget… #55257

goderbauer merged 3 commits into from
May 1, 2020

Conversation

@monkeyswarm
Copy link
Contributor

@monkeyswarm monkeyswarm commented Apr 21, 2020

…Details, to report offset on drop

Description

Adds the ability for DragTarget to get the drop offset. Adds a new onAcceptDetails callback, and helper class DragTargetDetails.

Related Issues

Tests

draggable_test.dart has been updated

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.

@fluttergithubbot fluttergithubbot added the framework flutter/packages/flutter repository. See also f: labels. label Apr 21, 2020
@kunit1
Copy link

kunit1 commented Apr 21, 2020

Exactly what I was looking for yesterday. Hope this is merged soon

goderbauer
goderbauer reviewed Apr 22, 2020
View reviewed changes
@monkeyswarm monkeyswarm force-pushed the DragTargetDetails branch 3 times, most recently from d7bf68d to 9a485ab Compare April 23, 2020 01:00
goderbauer
goderbauer reviewed Apr 23, 2020
View reviewed changes
@monkeyswarm
Copy link
Contributor Author

monkeyswarm commented Apr 28, 2020

Ping @goderbauer

@goderbauer
Copy link
Member

goderbauer commented Apr 29, 2020

@monkeyswarm My last comment about the location of the data still seems unresolved. Any thoughts on that?

goderbauer
goderbauer reviewed Apr 29, 2020
View reviewed changes
goderbauer
goderbauer reviewed Apr 29, 2020
View reviewed changes
@monkeyswarm
Copy link
Contributor Author

monkeyswarm commented Apr 30, 2020

Apologies, I hadn't pushed changes which moved the data into the DragTargetDetails. PTAL.

@goderbauer
Copy link
Member

goderbauer commented Apr 30, 2020

Looks like the analyzer is unhappy.

@monkeyswarm
Copy link
Contributor Author

monkeyswarm commented Apr 30, 2020

I noticed that, however, its complaints seem wrong. It's a bunch of 'always_specify_types' lint errors, but all of the lines already have fully specified types. It feels as if the analyzer is looking at a very old commit (as I had fixed this several commit-pushes ago). Is there a known issue with force-pushing commits? (I.e. should I be pushing new commits instead of force-pushing the same commit?)

goderbauer
goderbauer approved these changes Apr 30, 2020
View reviewed changes
Copy link
Member

@goderbauer goderbauer left a comment

Choose a reason for hiding this comment

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

LGTM

packages/flutter/lib/src/widgets/drag_target.dart Outdated
Copy link
Member

@goderbauer goderbauer Apr 30, 2020

Choose a reason for hiding this comment

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

nit:

Suggested change
widget.onAcceptWithDetails(DragTargetDetails(data:avatar.data as T, offset:avatar._lastOffset));
widget.onAcceptWithDetails(DragTargetDetails(data: avatar.data as T, offset: avatar._lastOffset));

@goderbauer
Copy link
Member

goderbauer commented Apr 30, 2020

Odd. Maybe push a new commit with the nit fixed to retrigger it?

@goderbauer
Copy link
Member

goderbauer commented Apr 30, 2020

FWIW, I am seeing the same analyzer failures when I run flutter analyze --flutter-repo locally over this PR. Those failures seem to be real. Please fix them.

@monkeyswarm
Copy link
Contributor Author

monkeyswarm commented Apr 30, 2020

I also see them when I run locally. But, for example, the second failure
info • Specify type annotations • test/widgets/draggable_test.dart:16:16 • always_specify_types
refers to this line
final List<DragTargetDetails> acceptedDetails = <DragTargetDetails>[];
which has fully specified type annotations (right)?

Is this a bug with the analyzer?

@goderbauer
Copy link
Member

goderbauer commented Apr 30, 2020

final List<DragTargetDetails> acceptedDetails = <DragTargetDetails>[];
which has fully specified type annotations (right)?

DragTargetDetails is defined as class DragTargetDetails<T> { ...}, so you will have to specify the T for the DragTargetDetails.

@googlebot
Copy link

googlebot commented Apr 30, 2020

All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter.

We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only @googlebot I consent. in this pull request.

Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the cla label to yes (if enabled on your project).

ℹ️ Googlers: Go here for more info.

@monkeyswarm
Copy link
Contributor Author

monkeyswarm commented Apr 30, 2020

, ah yes, forgot about the generics part of the specification, thanks.

@googlebot
Copy link

googlebot commented Apr 30, 2020

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

goderbauer
goderbauer approved these changes Apr 30, 2020
View reviewed changes
Copy link
Member

@goderbauer goderbauer left a comment

Choose a reason for hiding this comment

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

LGTM when cirrus is happy.

@goderbauer goderbauer merged commit 27eee14 into flutter:master May 1, 2020
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 31, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@goderbauer goderbauer goderbauer approved these changes

Assignees

No one assigned

Labels

framework flutter/packages/flutter repository. See also f: labels.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Expose drop offset in DragTarget

5 participants

@monkeyswarm @kunit1 @goderbauer @googlebot @fluttergithubbot