Improve Popup/Marker positioning logic #4620
Conversation
|
Just realized all this math is totally wrong if the map is rotated. 🤦♂️ |
src/ui/popup.js
Outdated
| if (this.options.closeOnClick) { | ||
| this._map.on('click', this._onClickClose); | ||
| } | ||
| this._update(); | ||
| this._update(false); |
There was a problem hiding this comment.
The problem with this is a marker added within -180 to 180 long might not be shown on the map initially.
eg. in this view australia is world -1 and I've added a marker at long 170 but it doesn't show up until I move the map.
There was a problem hiding this comment.
Agreed -- I've changed this to always apply the wrapping. Only the logic based on prior position is bypassed on the update immediately following setLatLng.
src/ui/marker.js
Outdated
| @@ -85,6 +86,7 @@ class Marker { | |||
| */ | |||
| setLngLat(lnglat) { | |||
| this._lngLat = LngLat.convert(lnglat); | |||
| this._wrappedLnLat = this._pos = null; | |||
There was a problem hiding this comment.
Let's use _wrappedLngLat for consistency.
There was a problem hiding this comment.
I decided to eliminate the variable entirely and just mutate this._lngLat .
src/ui/marker.js
Outdated
|
|
||
| if (this._map.transform.renderWorldCopies) { | ||
| this._wrappedLnLat = smartWrap(this._wrappedLnLat, this._pos, this._map.transform); | ||
| } |
There was a problem hiding this comment.
Could be shorter:
this._wrappedLngLat = this._map.transform.renderWorldCopies ?
smartWrap(this._lngLat, this._pos, this._map.transform) : this._lngLat;
|
I tested the updated branch, looks good to me. 👍 |
Wrap in a way that best preserves object constancy; i.e. minimizes perceived changes in position
Fixes #4577; see discussion there.
Launch Checklist