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

[google_maps_flutter_web] Add "My Location" Widget #6868

Closed
wants to merge 43 commits into from

Conversation

nploi
Copy link

@nploi nploi commented Dec 20, 2022

Currently, we don't "My Location" widget, so this PR i want support that feature, Thanks

Issue: flutter/flutter#64073

Pre-launch Checklist

  • I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • I read the Tree Hygiene wiki page, which explains my responsibilities.
  • I read and followed the relevant style guides and ran the auto-formatter. (Unlike the flutter/flutter repo, the flutter/plugins repo does use dart format.)
  • I signed the CLA.
  • The title of the PR starts with the name of the plugin surrounded by square brackets, e.g. [shared_preferences]
  • I listed at least one issue that this PR fixes in the description above.
  • I updated pubspec.yaml with an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.
  • I updated CHANGELOG.md to add a description of the change, following repository CHANGELOG style.
  • I updated/added relevant documentation (doc comments with ///).
  • I added new tests to check the change I am making, or this PR is test-exempt.
  • All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

@nploi
Copy link
Author

nploi commented Dec 20, 2022

small demo

demo-2.mov

@nploi nploi marked this pull request as ready for review December 25, 2022 09:57
@nploi nploi requested a review from ditman as a code owner December 25, 2022 09:57
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.

Some comments, sorting in degree of importance (IMO):

  • Use the watchPosition API so MyLocation automatically updates (instead of getCurrentPosition).
  • Handle the case where users deny Geolocation permissions to the app, so the app doesn't crash or freeze awaiting for a Future/Stream that will never complete.
  • Do not "hard-code" the translucent blue halo in the blue-dot.png, because that needs to be computed from the coords.accuracy value that the Geolocation API returns. Blue dot should be a blue circle with a white border and nothing more.
  • Speaking of assets: make the mylocation-sprite-2x.png part of the assets of the package, and do not download it from a gstatic URL.
  • Move (most of) the code to a separate src/my_location.dart file to not bloat the MapsController code, and also to make it easier to test. Pass parameters for the things that you need (like the gmaps.GMap or the MarkersController)
  • Try to make the "loading" animation (and the "done" state) come from CSS, rather than using JS to build those animations (I provided a jsfiddle example).
  • Some comments around naming that are completely subjective, but try to make the name of the method describe what's happening inside of it (for example, currently: _moveToCurrentLocation is a little bit weird)

Thanks for the contribution, very exciting to see this land!

// Get current location
Future<LatLng> _getCurrentLocation() async {
final Geoposition location =
await window.navigator.geolocation.getCurrentPosition();
Copy link
Member

Choose a reason for hiding this comment

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

This method will not update the user's location if they move around, unless something else on the map changes. I think this should use the watchPosition API, that returns a Stream<Geoposition> and then respond to the events from that stream. Some changes to this PR:

  • If the user wants to render the MyLocation dot: subscribe to the watchPosition Stream, and on each Geolocation event update the Marker.
    • The first event of the watchPosition Stream can be used to remove the "waiting" animation class from the button, for example, and to get ready the marker that needs to be rendered.
  • If the user wants to stop rendering their location, it's easy to remove whatever subscription to the Stream that we do, and whatever cleanup is needed.

Some documentation:

@nploi nploi marked this pull request as draft December 31, 2022 11:48
@nploi
Copy link
Author

nploi commented Jan 1, 2023

Hi @ditman, can you help to review ?, i'm fixed some your comments, but there is a comment i will fix it in the next pr because this pr is too big to review :D.

@nploi nploi marked this pull request as ready for review January 1, 2023 11:03
@nploi nploi requested a review from ditman January 1, 2023 11:06
@stuartmorgan-g
Copy link
Contributor

Update from triage: this in on @ditman's radar, but there's been high-priority work in another plugin that has taken precedence.

@nploi
Copy link
Author

nploi commented Feb 10, 2023

Update from triage: this in on @ditman's radar, but there's been high-priority work in another plugin that has taken precedence.

@stuartmorgan Thank you for letting me know.

@stuartmorgan-g
Copy link
Contributor

We've just completed the migration of the plugin code to the flutter/packages repository, as described in https://flutter.dev/go/flutter-plugins-repo-migration, and this repository is now being archived. Unfortunately that means that all in-progress PRs here must be moved to flutter/packages.

Please see our instructions for an explanation of how to move your PR, and if you have any issues moving your PR please don't hesitate to reach out in the #hackers-ecosystem channel in Discord.

Our apologies that your PR was caught in this one-time transition. We're aware that it's disruptive in the short term, and appreciate your help in getting us to a better long-term state!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants