add popup offset option

[andrewharvey]

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.

[mollymerp]

Thanks for contributing @andrewharvey! I will take some time to review this PR but in the meantime could you add unit tests ?

[andrewharvey]

Thanks for contributing @andrewharvey! I will take some time to review this PR but in the meantime could you add unit tests ?

@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
http://bl.ocks.org/andrewharvey/b707124c705ea64a07bdd50396433fe8
http://bl.ocks.org/andrewharvey/db2e3421b8f0c58be06cdfff156527ae

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?

[mollymerp]

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 offset implementation makes sense to me -- @lucaswoj do you want to take a look and confirm?

[lucaswoj]

Thanks for the PR @andrewharvey!

I'm uncomfortable with the complexity of this API.

What use cases do we cover with the anchor: string and offset: number|PointLike|Object API that we couldn't cover with a simple Leaflet-style anchor: PointLike API?

[andrewharvey]

@lucaswoj Thanks for reviewing.

I'm uncomfortable with the complexity of this API.

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.

What use cases do we cover with the anchor: string and offset: number|PointLike|Object API that we couldn't cover with a simple Leaflet-style anchor: PointLike API?

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:

1. http://bl.ocks.org/andrewharvey/60c0b1f12118bda230174ff630931278
2. http://bl.ocks.org/andrewharvey/b707124c705ea64a07bdd50396433fe8
3. http://bl.ocks.org/andrewharvey/db2e3421b8f0c58be06cdfff156527ae

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)

[andrewharvey]

Any progress here? I'm not sure what else I can provide to assist.

[jfirebaugh]

My 2¢: Given the auto-anchoring behavior of Popup, I think @andrewharvey is correct that this level of flexibility is desirable. The changes here look good to me.

[lucaswoj]

🚢 in #3058!

[therobwatson]

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;
}