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

[google_maps_flutter_web] Implement my location & my location button #4486

Closed
wants to merge 7 commits into from

Conversation

nploi
Copy link

@nploi nploi commented Jul 16, 2023

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

Issue: flutter/flutter#64073

Recreating PR from flutter/plugins from flutter/plugins#6868

Demo:

Screen.Recording.2023-07-28.at.23.55.03.mov

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/packages repo does use dart format.)
  • I signed the CLA.
  • The title of the PR starts with the name of the package 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 nploi marked this pull request as ready for review July 28, 2023 16:58
@nploi nploi requested a review from ditman as a code owner July 28, 2023 16:58
@stuartmorgan
Copy link
Contributor

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.)

@ditman
Copy link
Member

ditman commented Sep 14, 2023

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.

@stuartmorgan
Copy link
Contributor

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 🙂

@ditman
Copy link
Member

ditman commented Sep 14, 2023

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?)

@stuartmorgan
Copy link
Contributor

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
Copy link
Contributor

@ditman Ping on this review.

@mariopepe
Copy link

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!

@ditman
Copy link
Member

ditman commented Feb 13, 2024

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!)

@stuartmorgan
Copy link
Contributor

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

@JPFrancoia
Copy link

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!

@stuartmorgan
Copy link
Contributor

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

@nploi
Copy link
Author

nploi commented May 1, 2024

@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.

@stuartmorgan stuartmorgan marked this pull request as draft June 25, 2024 19:59
@stuartmorgan
Copy link
Contributor

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

@eifr
Copy link

eifr commented Jul 2, 2024

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

@tokuyamamitsushi

This comment was marked as off-topic.

@Piinks
Copy link
Contributor

Piinks commented Sep 24, 2024

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

(PR triage): Hey @stuartmorgan, is this still waiting on that work?

@stuartmorgan
Copy link
Contributor

Since this is marked as a draft and hasn't been updated in several months I'm going to close it to clean out our review queue. Please don't hesitate to submit a new PR if you decide to revisit this. Thanks!

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

Successfully merging this pull request may close these issues.

8 participants