Skip to content
This repository has been archived by the owner on Feb 22, 2023. It is now read-only.

[google_maps_flutter] Marker dragging events #2653

Merged
merged 1 commit into from
Sep 24, 2021
Merged

[google_maps_flutter] Marker dragging events #2653

merged 1 commit into from
Sep 24, 2021

Conversation

JamesMcIntosh
Copy link
Contributor

@JamesMcIntosh JamesMcIntosh commented Apr 15, 2020 •
edited
Loading

Description

Add events to track start of drag and position during drag to platform interface

Implementation in PR: #2838

Related Issues

flutter/flutter#26117

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.
  • My PR includes unit or integration tests for all changed/updated/fixed behaviors (See [Contributor Guide]).
  • All existing and new tests are passing.
  • I updated/added relevant documentation (doc comments with ///).
  • The analyzer (flutter analyze) does not report any problems on my PR.
  • I read and followed the [Flutter Style Guide].
  • The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
  • I updated pubspec.yaml with an appropriate new version according to the [pub versioning philosophy].
  • I updated CHANGELOG.md to add a description of the change.
  • I signed the [CLA].
  • I am willing to follow-up on review comments in a timely manner.

Breaking Change

Does your PR require plugin users to manually update their apps to accommodate your change?

  • Yes, this is a breaking change (please indicate a breaking change in CHANGELOG.md and increment major revision).
  • No, this is not a breaking change.

@JamesMcIntosh JamesMcIntosh force-pushed the google-maps-marker-drag-events branch from 7d49770 to 18381e1 Compare April 15, 2020 03:59
@ened
Copy link
Contributor

ened commented Apr 18, 2020

@JamesMcIntosh nice PR.. could you rebase & perhaps add a small demo in the example app?

cyanglaz
cyanglaz previously requested changes Apr 21, 2020
View reviewed changes
Copy link
Contributor

@cyanglaz cyanglaz left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. It mostly looks good, I left some comments.

packages/google_maps_flutter/google_maps_flutter/CHANGELOG.md Outdated Show resolved Hide resolved
packages/google_maps_flutter/google_maps_flutter/lib/src/marker.dart Outdated Show resolved Hide resolved
packages/google_maps_flutter/google_maps_flutter/pubspec.yaml Outdated Show resolved Hide resolved
@cyanglaz
Copy link
Contributor

@JamesMcIntosh nice PR.. could you rebase & perhaps add a small demo in the example app?

+1

ditman
ditman suggested changes Apr 21, 2020
View reviewed changes
Copy link
Member

@ditman ditman left a comment

Choose a reason for hiding this comment

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

Hello there! Thank you very much for the PR!

We're about to merge a change to google_maps_flutter, so it follows the "federated implementation" style. This will allow new platforms (like web) to be supported.

Once that change lands, some of the code that you've created/touched should live in the google_maps_flutter_platform_interface package, and not here.

Check out this PR on how we migrated the whole plugin to the new architecture. It should give you clues on how to modify your code to conform to it.

Feel free to reach out to me if you need any assistance with this!

packages/google_maps_flutter/google_maps_flutter/lib/src/controller.dart Outdated Show resolved Hide resolved
@ditman
Copy link
Member

ditman commented Apr 24, 2020

The refactor has landed.

@maxim-toleutai
Copy link

is it work on iOs?

@JamesMcIntosh JamesMcIntosh force-pushed the google-maps-marker-drag-events branch 5 times, most recently from 59c4987 to 2ca3d63 Compare June 18, 2020 02:45
@JamesMcIntosh
Copy link
Contributor Author

JamesMcIntosh commented Jun 18, 2020 •
edited
Loading

@ditman I can't get the build to work properly without directly specifying google_maps_flutter_platform_interface path in the google_maps_flutter pubspec.yaml.
This allows the build to compile artifacts properly but the publishable check to fail as it does not allow paths.
* Don't depend on "google_maps_flutter_platform_interface" from the path source. Use the hosted source instead....

The problem I think is because there are changes in thegoogle_maps_flutter_platform_interface as well as the google_maps_flutter.

When I tried without the path it fails because I have bumped the version for google_maps_flutter_platform_interface and referenced it in google_maps_flutter but it cannot be resolved because it has not been published.

@ditman
Copy link
Member

ditman commented Jun 18, 2020

@JamesMcIntosh that is correct, you need to land your changes in a package-by-package basis. I'd recommend that you split your changes to the platform_interface package to a separate PR, and then refresh this PR once your platform_interface package changes land. That way you'll be able to specify a version dependency here.

@JamesMcIntosh JamesMcIntosh force-pushed the google-maps-marker-drag-events branch from 2ca3d63 to c3ab6e7 Compare June 19, 2020 02:01
@JamesMcIntosh JamesMcIntosh mentioned this pull request Jun 19, 2020
Merged
13 tasks
@JamesMcIntosh
Copy link
Contributor Author

JamesMcIntosh commented Jun 19, 2020 •
edited
Loading

@ditman Thanks David, I've split it up. Implementation in new PR
#2838

@JamesMcIntosh JamesMcIntosh force-pushed the google-maps-marker-drag-events branch from c3ab6e7 to a71d033 Compare September 10, 2020 02:49
@JamesMcIntosh
Copy link
Contributor Author

Hi @ditman @cyanglaz, How are you guys looking at getting this PR re-reviewed and merged?
Many thanks
James

@JamesMcIntosh
Copy link
Contributor Author

Hi @cyanglaz & @ditman, just wondering where you are at with getting this re-reviewed and the associated implementation merged? Would be nice to get back onto the plugin release rather than having to keep tracking the branch.

@JamesMcIntosh JamesMcIntosh force-pushed the google-maps-marker-drag-events branch from a71d033 to 08e092a Compare October 14, 2020 20:52
@JamesMcIntosh
Copy link
Contributor Author

Hi @cyanglaz @ditman, it would be great if I could have a response as it's now been 2 months

@shujaatak
Copy link

It's a much needed feature. @JamesMcIntosh you have done a great work.

@stuartmorgan
Copy link
Contributor

We apologize for the long delay in getting back to this PR. We’re in the process of overhauling our PR triage system to respond much more quickly in the future, as well as working through the backlog.

We're in the middle of doing null-safety migration for this plugin, but once that is landed @ditman will work with you on getting this reviewed.

@shujaatak
Copy link

@JamesMcIntosh @ditman Marker dragging events is a much needed feature. We wish and hope it become a part of the google maps flutter as soon as possible.

@JamesMcIntosh JamesMcIntosh force-pushed the google-maps-marker-drag-events branch from 08e092a to 72ca4b1 Compare April 26, 2021 12:20
@JamesMcIntosh
Copy link
Contributor Author

Hi @ditman, I've migrated the code in the PR to be null safe, are you able to review it please?

@JamesMcIntosh JamesMcIntosh force-pushed the google-maps-marker-drag-events branch 2 times, most recently from dbc0c82 to dad021e Compare July 25, 2021 13:03
@JamesMcIntosh
Copy link
Contributor Author

Hi Sebastian @ened,
Could you please review my changes based on your comments and merge this and the corresponding PR #2838?
Many thanks
James

@JamesMcIntosh JamesMcIntosh force-pushed the google-maps-marker-drag-events branch 4 times, most recently from d3e0900 to d020db4 Compare September 21, 2021 14:29
@JamesMcIntosh
Copy link
Contributor Author

Hi @stuartmorgan, I have added the requested tests.
The checks are failing the federated_safety check, not sure why as all changes are self contained with the platform_interface package.

@stuartmorgan
Copy link
Contributor

The checks are failing the federated_safety check, not sure why as all changes are self contained with the platform_interface package.

Because the check is totally broken 🤦🏻

I'll have a fix posted shortly.

@stuartmorgan
Copy link
Contributor

stuartmorgan commented Sep 21, 2021 •
edited
Loading

Once #4368 lands you'll need to merge/rebase, and then it should work. Sorry for the hassle, and thanks for flagging it right away.

@JamesMcIntosh JamesMcIntosh force-pushed the google-maps-marker-drag-events branch from d020db4 to 0ce2216 Compare September 22, 2021 11:32
@ditman
Copy link
Member

ditman commented Sep 22, 2021

Taking a look right now.

ditman
ditman approved these changes Sep 24, 2021
View reviewed changes
Copy link
Member

@ditman ditman left a comment

Choose a reason for hiding this comment

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

LGTM, let's land this! Thanks for the extra coverage on the marker type!

@ditman
Copy link
Member

ditman commented Sep 24, 2021

The submit-queue action looks red, but I checked flutter/master and all is green. I'm landing this now, and will keep an eye on CI tonight.

@ditman ditman dismissed cyanglaz’s stale review September 24, 2021 01:41

All requested changes were actually done!

@ditman ditman merged commit a5cd0c3 into flutter:master Sep 24, 2021
@engine-flutter-autoroll engine-flutter-autoroll mentioned this pull request Sep 24, 2021
Merged
engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Sep 24, 2021
@engine-flutter-autoroll
a5cd0c3 [google_maps_flutter_platfomr_interface] Add Marker drag even…
0f65b65 
…ts (flutter/plugins#2653)
fluttergithubbot pushed a commit to flutter/flutter that referenced this pull request Sep 24, 2021
@engine-flutter-autoroll
a5cd0c3 [google_maps_flutter_platfomr_interface] Add Marker drag even…
38a19ce 
…ts (flutter/plugins#2653) (#90646)
@JamesMcIntosh JamesMcIntosh mentioned this pull request Sep 25, 2021
Merged
amantoux pushed a commit to amantoux/plugins that referenced this pull request Sep 27, 2021
@JamesMcIntosh @amantoux
[google_maps_flutter_platfomr_interface] Add Marker drag events (flut…
73e0eb8 
…ter#2653)

This PR adds onDragStart(LatLng) and onDrag(LatLng) events to Marker objects, in addition to the already existing onDragEnd.
NickalasB added a commit to NickalasB/plugins that referenced this pull request Sep 27, 2021
@NickalasB
Merge branch 'master' into add_zoom_to_android_webview
8dd870b 
* master: (51 commits)
  [webview_flutter] Update version number app_facing package (flutter#4375)
  [webview_flutter] Adjust integration test domains (flutter#4383)
  Remove some trivial custom analysis options files (flutter#4379)
  [google_maps_flutter_platfomr_interface] Add Marker drag events (flutter#2653)
  [flutter_plugin_tools] Improve version check error handling (flutter#4376)
  [flutter_plugin_tools] Allow overriding breaking change check (flutter#4369)
  [url_launcher] Error handling when URL cannot be parsed with Uri.parse (flutter#4365)
  [webview_flutter] Migrate main package to fully federated architecture. (flutter#4366)
  [google_sign_in] Bump minimum Flutter version and iOS deployment target (flutter#4334)
  Add false secret lists, and enforce ordering (flutter#4372)
  [camera_web] Update usage documentation (flutter#4371)
  [video_player] VTT Support (flutter#2878)
  Require authors file (flutter#4367)
  [flutter_plugin_tools] Fix federated safety check (flutter#4368)
  [webview_flutter] Extract WKWebView implementation into a separate package (flutter#4345)
  [webview_flutter] Extract Android implementation into a separate package (flutter#4343)
  [in_app_purchase] Ensure the `introductoryPriceMicros` field is populated correctly. (flutter#4364)
  [flutter_plugin_tools] Add a federated PR safety check (flutter#4329)
  [camera] Add web support (flutter#4240)
  [webview_flutter] Bump minimum Flutter version and iOS deployment target (flutter#4361)
  ...

# Conflicts:
#	packages/webview_flutter/webview_flutter/lib/platform_interface.dart
#	packages/webview_flutter/webview_flutter/lib/src/webview_method_channel.dart
#	packages/webview_flutter/webview_flutter/lib/webview_flutter.dart
clocksmith pushed a commit to clocksmith/flutter that referenced this pull request Oct 29, 2021
@engine-flutter-autoroll @clocksmith
a5cd0c3 [google_maps_flutter_platfomr_interface] Add Marker drag even…
f59f3d5 
…ts (flutter/plugins#2653) (flutter#90646)
KyleFin pushed a commit to KyleFin/plugins that referenced this pull request Dec 21, 2021
@JamesMcIntosh @KyleFin
[google_maps_flutter_platfomr_interface] Add Marker drag events (flut…
27d3ea1 
…ter#2653)

This PR adds onDragStart(LatLng) and onDrag(LatLng) events to Marker objects, in addition to the already existing onDragEnd.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@ened ened ened left review comments

@ditman ditman ditman approved these changes

@cyanglaz cyanglaz Awaiting requested review from cyanglaz

Assignees
No one assigned
Labels
cla: yes needs tests p: google_maps_flutter
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
8 participants
@JamesMcIntosh @ened @cyanglaz @ditman @maxim-toleutai @shujaatak @stuartmorgan @googlebot