-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
add popup offset option #2906
add popup offset option #2906
Conversation
2c35d09
to
8ac4cb6
Compare
Thanks for contributing @andrewharvey! I will take some time to review this PR but in the meantime could you add unit tests ? |
8ac4cb6
to
9229a38
Compare
@mollymerp What do you think will work best for the unit tests? Given that currently there appears to be no unit tests for popups but only a manual test page https://github.com/mapbox/mapbox-gl-js/blob/master/test/manual/popup.html The tests I wrote for the development are: http://bl.ocks.org/andrewharvey/60c0b1f12118bda230174ff630931278 They cover the single xy, radius and multiple xy cases respectively. For the latter two the manual test involved panning the map to ensure that when the popup anchor changes (to avoid the popup being outside the map container) that the tip is always at the right location, and that the anchor changes at the correct time (also ensuring the popup never falls outside the map container). I've gone ahead and added tests in similar fashion as the current manual popup test, is this enough? |
Thanks for the test cases @andrewharvey ! You're right, we don't currently have a good way to test for user interactions and DOM events. This is definitely something I would like to explore further in the future. Your |
Thanks for the PR @andrewharvey! I'm uncomfortable with the complexity of this API. What use cases do we cover with the |
@lucaswoj Thanks for reviewing.
I've taken a lot of care to make this as simple as possible, while still covering the major use cases I deem necessary for a better end user user experience.
A GL JS Popup by default will change the anchor to ensure it remains within the map, like in this example when you pan the map the popup anchor will change. As soon as you introduce a popup offset, that offset will likely be different for each anchor. I believe the three demos I put together show the use cases:
A simple PointLike offset wouldn't be able to take into account the different offsets need for each anchor in (2) and (3), we need to support an Object of PointLikes for (3). In theory we could just support an Object of PointLikes for all these cases, but I believe it's simpler to support all 3 options. So that for case (1) you only need to pass one PointLike, and for the very common case of a circular marker (2) you only need to pass a single number, which is much simpler than always having to pass an object full of PointLikes for each anchor I think. This approach was inspired by the suggestion at #1962 (comment) |
Any progress here? I'm not sure what else I can provide to assist. |
My 2¢: Given the auto-anchoring behavior of |
🚢 in #3058! |
…etween Marker and Popup (mapbox/mapbox-gl-js#2906) so we use that to make sure the marker image is always visible (Mapbox puts the popup on whichever side has most room for it, offset appropriately). This commit does the following: (1) get the latest mapbox, then (2) put a half-circle-width radius on the popups, and (3) recenter the marker image circles so the center of the image is over the lat/long we're adding.
EDIT: the following hack can produce the unwanted result of making the popup rapidly flip between being above and below the marker. Back to the drawing board! Here is @cvn 's idea (from issue 1962) without using jQuery (adapted from @cyprianos 's idea in issue 2848: var popupContent = document.createElement("div");
popupContent.innerHTML = "<p>Hello World</p>";
/* if using a vertical marker like a standard map "pin", do not specify an offset
(using a class below instead, "eventMarkerVerticalOffset"). Mapbox offset only
supports offsets like the element triggering the popup is a circle of some sort.
That is, vertical (call it north, "N") offset "works", but NE, E, SE, S, SW, W,
and NW offsets are messed up for your standard vertical "pin" marker */
this.popups.push(new mapboxgl.Popup({ offset: 0 })
.setLngLat(feature.geometry.coordinates)
.setDOMContent(popupContent)
.addTo(e.target));
// append a class for a specific vertical offset
popupContent.parentNode.parentNode.className += " eventMarkerVerticalOffset"; And then in the CSS, specify a parent wrapper div (e.g. "div.hello-world-mapbox-map") in order to override Mapbox's CSS, if needed, and apply the vertical offset you wanted to the "anchor-bottom" element.: div.hello-world-mapbox-map .mapboxgl-popup-anchor-bottom.vehicleMarkerVerticalOffset .mapboxgl-popup-tip {
margin-bottom: 90px;
} |
I've tried to balance simplicity without reducing functionality for these use cases:
http://bl.ocks.org/andrewharvey/60c0b1f12118bda230174ff630931278
http://bl.ocks.org/andrewharvey/b707124c705ea64a07bdd50396433fe8
http://bl.ocks.org/andrewharvey/db2e3421b8f0c58be06cdfff156527ae
comments/feedback most welcome.
One limitation is that trying to offset a circle which has a size based on zoom won't work.