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

fitToMarkers resists dragging #69

Closed
livtanong opened this issue Nov 13, 2014 · 10 comments · Fixed by #72
Closed

fitToMarkers resists dragging #69

livtanong opened this issue Nov 13, 2014 · 10 comments · Fixed by #72

Comments

@livtanong
Copy link
Contributor

This is pretty weird: when I drag, the screen shakes wildly, because the viewport is moved by a bit, then reset back. If it really IS supposed to resist the drag, then I'd recommend to include in the implementation setOptions({ draggable: false }) to prevent the shaking. I'd also suggest the addition of a fitToMarkers method that resets the view once, and can be called again at a later time to reset the view after the user has navigated away.

@ebidel
Copy link
Contributor

ebidel commented Nov 16, 2014

Surprised I never ran into this!I can repo it on http://googlewebcomponents.github.io/google-sheets/components/google-sheets/demo.html.

The intention is that the user can still drag the map. Looks like this is from https://github.com/GoogleWebComponents/google-map/blob/master/google-map.html#L668:L669. The lat/lng get updated and updateCenter() gets called as a result. This calls showCenterMarkerChanged() explicitly which does the fitToMakers stuff. There was a good reason for that call but I can't recall off the top of my head.

Interested in submitting a PR?

@livtanong
Copy link
Contributor Author

Alright, I've made a PR! It's a simple fix, and it partially solves the issue. It will still explode if showCenterMarker is true, but that's a separate discussion on what you want the behavior to be.

The two are mutually exclusive I think. I mean, you have a marker that's supposed to stay in the center of your map all the time, and if you have other markers on the map that will change the map center position, then you'll get some pretty weird stuff.

I have three recommendations off the top of my head:

  1. We can make it so that one overrides the other, and mention it in the documentation. e.g. "fitToMarkers will override showCenterMarker, whenever it's set to true." In which case a true value in one will automatically turn the other false.
  2. Throw an error if the two mutually exclusive cases are set to true, and again, mention it in the documentation.
  3. Do not include the centerMarker in calculations for fitToMarkers. This may sound good and correct, but it's a special case may complicate things, and we may get more weird edge cases from this.

@aidanlister
Copy link

+1

@eximius313
Copy link
Contributor

I'm also experiencing the same issue!
map shakes and doesn't allow to drag it (even when zoomed in)

@rbrewer101
Copy link

Me too.
Any estimate of if/when this little weirdness might be resolved?
I'm not quite up to the task of fixing it myself, but offer gratitude to anyone who does get it resolved.

@livtanong
Copy link
Contributor Author

To quickly fix this, have a look at google-map.html. There'll be this.updateMarkers();. Wrap it in a conditional hinged on the existence of centerMarker.

if (this.centerMarker) {
  this.updateMarkers();
}

@ebidel ebidel closed this as completed in #72 Mar 2, 2015
@ebidel
Copy link
Contributor

ebidel commented Mar 2, 2015

@levitanong I merged your PR. Thanks! I think your #3 makes sense. It may confuse users if the center marker is treated differently, but it's always been more of a convenience than anything else. It also existed before we had proper marker support, so maybe it's time to actually remove it.

@livtanong
Copy link
Contributor Author

Hurrah!

@eximius313
Copy link
Contributor

I'd say that #2 is more verbose than #1 (fail fast) and we don't introduce additional complexity as in #3

@rbrewer101
Copy link

Works! Thanks very much @levitanong and @ebidel.

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

Successfully merging a pull request may close this issue.

5 participants