packages icon indicating copy to clipboard operation
packages copied to clipboard

[google_maps_flutter_web] Implement my location & my location button

Open nploi opened this issue 2 years ago • 16 comments

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

Issue: https://github.com/flutter/flutter/issues/64073

Recreating PR from flutter/plugins from https://github.com/flutter/plugins/pull/6868

Demo:

https://github.com/flutter/packages/assets/34020090/e966dbde-04a7-4778-b6fe-e03140f3ed39

Pre-launch Checklist

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

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

nploi avatar Jul 16 '23 14:07 nploi

Update from triage: @ditman is currently OOO

(@ditman, when you get back to this review, one high-level question: do we want to have the polyfill in the plugin, vs exposing the location API to allow this code to live as a separate third-party package to do a polyfill? We usually just wrap the functionality of the native SDKs.)

stuartmorgan-g avatar Aug 24 '23 16:08 stuartmorgan-g

do we want to have the polyfill in the plugin, vs exposing the location API to allow this code to live as a separate third-party package to do a polyfill? We usually just wrap the functionality of the native SDKs.

@stuartmorgan do you mean the Geolocation/Geoposition classes, or the whole shebang of the MyLocation widget, the button on the map and friends?

The former are built-ins in the browser (see Geolocation API), and already "externalized" because they're provided by dart:html (or package:web in the near future).

(Another alternative if we wanted to use an external geo-location client could be to rely on package:geolocator? but that might be too much extra code for what this plugin needs).

The latter might be externalizable, but then that package would end up being something that just renders a bunch of HTML elements that can only be used from our Maps plugin.

ditman avatar Sep 14 '23 01:09 ditman

The former are built-ins in the browser (see Geolocation API), and already "externalized" because they're provided by dart:html (or package:web in the near future).

Ah, I thought this was a maps API for some reason.

So then my question would be: is there anything that would prevent this entire PR from instead being a separate package that people who want functionality that isn't in the SDK could use? (And if so, could we reasonably add a small set of APIs to fix that?)

The latter might be externalizable, but then that package would end up being something that just renders a bunch of HTML elements that can only be used from our Maps plugin.

Wouldn't it be a package that gets your location and adds HTML elements based on it, which could be used with any map plugin, not just ours?

But even if it's plugin specific, is that a problem? If people using our Maps plugin and who want specific extras functionality can install a third-party package to get it, that seems like the ecosystem working as intended 🙂

stuartmorgan-g avatar Sep 14 '23 10:09 stuartmorgan-g

But even if it's plugin specific, is that a problem? If people using our Maps plugin and who want specific extras functionality can install a third-party package to get it, that seems like the ecosystem working as intended 🙂

No, in fact, it'd be nice if the google maps plugin allowed for this. The main missing feature I see now is "creating new UI elements in the map Widget to add new behavior" (in this case, what the "_addMyLocationButton" method is doing).

Rendering the "position" itself seems to be just rendering an extra Marker and keep its position updated, so that'd be easy to inject (just give users a method to inject the position marker into the list of markers that they want to render, for example).

One of the issues here is that this feature is built-in in the mobile version of the maps (probably the last "big" feature remaining on the web from the OG feature set of the plugin?)

ditman avatar Sep 14 '23 21:09 ditman

One of the issues here is that this feature is built-in in the mobile version of the maps

It's your call if you want to own the polyfill. I tend to view the role of a plugin like this to be more "expose the SDK features to Flutter", which means functionality that's missing on one platform is missing, but it's definitely not a bright line.

One of the issues here is that this feature is built-in in the mobile version of the maps

Sure, but presumably non-Flutter web devs also view that as an issue and make stand-alone polyfills? 🤷 Anyway, up to you.

stuartmorgan-g avatar Sep 14 '23 22:09 stuartmorgan-g

@ditman Ping on this review.

stuartmorgan-g avatar Oct 10 '23 19:10 stuartmorgan-g

hey @nploi congrats for this PR, this would be such a good addition for Google Maps Web.

Do you know if this will be merged? Did @ditman had the chance to review it?

Thanks again!

mariopepe avatar Dec 10 '23 02:12 mariopepe

There's an incoming migration to package:web that would make this PR more complicated, let's have that PR land first, and then I'll update this one with the latest code, and some changes that I've seen (for example, the way that we compute the asset URL for the button is not great!)

ditman avatar Feb 13 '24 23:02 ditman

It looks like the linked PR has landed; is this ready to be updated then? @ditman

stuartmorgan-g avatar Mar 05 '24 20:03 stuartmorgan-g

screen-2024-03-16-15-08-13

This PR works for me (on Chrome and Archlinux, I'll test on chrome/Firefox on Android soon). Just had to pin a couple of dependencies to get it to run, but that's all. @nploi , thank you, I appreciate your hard work. Any chance you could merge master in your branch to make this code more up to date? It moved a bit since you opened this PR.

Again, thank you so much!

JPFrancoia avatar Mar 16 '24 18:03 JPFrancoia

@nploi Are you still planning on updating this PR per the discussion above?

stuartmorgan-g avatar Apr 30 '24 19:04 stuartmorgan-g

@nploi Are you still planning on updating this PR per the discussion above?

@stuartmorgan yes, I will update this pr soon with the above discussion.

nploi avatar May 01 '24 04:05 nploi

Marking as a draft for now, pending the updates to use package:web being integrated.

stuartmorgan-g avatar Jun 25 '24 19:06 stuartmorgan-g

Marking as a draft for now, pending the updates to use package:web being integrated.

could you refer which updates are pending? i'd like to follow

eifr avatar Jul 02 '24 08:07 eifr

@ditman This pull request #4486 contains important library updates that I need to use in my project. Please respond promptly to this request. Thank you in advance for your understanding and cooperation.

tokuyamamitsushi avatar Jul 19 '24 06:07 tokuyamamitsushi