-
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
WIP: added support for anchor option on Marker #6031
Changes from 9 commits
19d42db
5ad5789
47d1243
3da1c0d
ada8e92
a9791fc
2270123
e0972bb
dcfc710
532b453
c4e2aca
278ed67
c3b9400
7211a29
c938fbd
9996b21
df4308a
fae2681
c8d0bd1
4275456
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,26 +11,54 @@ import type Popup from './popup'; | |
import type {LngLatLike} from "../geo/lng_lat"; | ||
import type {MapMouseEvent} from './events'; | ||
|
||
export type Anchor = 'top' | 'bottom' | 'left' | 'right' | 'top-left' | 'top-right' | 'bottom-left' | 'bottom-right'; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
export type Offset = number | PointLike | {[Anchor]: PointLike}; | ||
|
||
export type MarkerOptions = { | ||
anchor: Anchor, | ||
offset: Offset | ||
} | ||
|
||
/** | ||
* Creates a marker component | ||
* @param element DOM element to use as a marker. If left unspecified a default SVG will be created as the DOM element to use. | ||
* @param options | ||
* @param options.offset The offset in pixels as a {@link PointLike} object to apply relative to the element's center. Negatives indicate left and up. | ||
* @param {Object} [element] DOM element to use as a marker. If left unspecified a default SVG will be created as the DOM element to use. | ||
* @param {Object} [options] | ||
* @param {string} [options.anchor] - A string indicating the markers's location relative to | ||
* the coordinate set via {@link Marker#setLngLat}. | ||
* Options are `'top'`, `'bottom'`, `'left'`, `'right'`, `'top-left'`, | ||
* `'top-right'`, `'bottom-left'`, and `'bottom-right'`. If unset the anchor will be | ||
* dynamically set to ensure the marker falls within the map container with a preference | ||
* for `'bottom'`. | ||
* @param {number|PointLike|Object} [options.offset] The offset in pixels as a {@link PointLike} object to apply relative to the element's center. Negatives indicate left and up. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This will now be "relative to the anchor" There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
I think with the anchor option it's clearer to say relative to the element's anchor. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Whops, sorry didn't see @anandthakker's comment There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i'm just seeing those as well X_x thank you both for taking the time to review and for your patience! @anandthakker i'm going through your comments and going to push the requested changes shortly :) |
||
* @example | ||
* var markerHeight = 50, markerRadius = 10, linearOffset = 25; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Even if it ends up supporting the same dynamic anchor logic as Popup, I don't think such a complicated use case should be the default example. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this is a good point, i'll make an example based on defaults |
||
* var markerOffsets = { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Popup has this since the anchor of the popup changes to ensure it's visible within the map (ie. moves below the point when there is no space above it), the Popup offset needs to be different for each anchor in some use cases. Is there a use case for having a variable anchor for a single marker and hence needing different offsets set for each anchor? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. mmm I was thinking about exposing as a variable so users could define their own offset per marker/anchor. This could be for styling considerations on static maps or maps with markers in close proximity to eachother. One possible route to take would have popup inherit the anchor from marker as a default for when users don't define? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see, for example the default Marker in Mapbox with an anchor of bottom still needs an offset due to the shadow, so if you have a marker that goes upsidedown when it can't fit at the top of the map and it has a shadow it needs an offset per anchor value. Got it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yup! I think its good to have built in flexibility but include batteries for desired behaviors if not specified. This was a good question, if you see anything else that sticks out would love to discuss. Thanks for taking the time to review my work so far ✨🎷🐛 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As @andrewharvey noted above, the reason we need this in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. sorry to keep things held up!! i've been working my way through testing and will make another commit soon but would definitely welcome another pair of eyes on this n_n |
||
* 'top': [0, 0], | ||
* 'top-left': [0,0], | ||
* 'top-right': [0,0], | ||
* 'bottom': [0, -markerHeight], | ||
* 'bottom-left': [linearOffset, (markerHeight - markerRadius + linearOffset) * -1], | ||
* 'bottom-right': [-linearOffset, (markerHeight - markerRadius + linearOffset) * -1], | ||
* 'left': [markerRadius, (markerHeight - markerRadius) * -1], | ||
* 'right': [-markerRadius, (markerHeight - markerRadius) * -1] | ||
* }; | ||
* var marker = new mapboxgl.Marker() | ||
* .setLngLat([30.5, 50.5]) | ||
* .addTo(map); | ||
* @see [Add custom icons with Markers](https://www.mapbox.com/mapbox-gl-js/example/custom-marker-icons/) | ||
*/ | ||
class Marker { | ||
_map: Map; | ||
options: MarkerOptions; | ||
_anchor: Anchor; | ||
_offset: Point; | ||
_element: HTMLElement; | ||
_popup: ?Popup; | ||
_lngLat: LngLat; | ||
_pos: ?Point; | ||
|
||
constructor(element: ?HTMLElement, options?: {offset: PointLike}) { | ||
constructor(element: ?HTMLElement, options?: {anchor: Anchor, offset: PointLike}) { | ||
this._anchor = Point.convert(options && options.anchor || [0, 0]); | ||
this._offset = Point.convert(options && options.offset || [0, 0]); | ||
|
||
bindAll(['_update', '_onMapClick'], this); | ||
|
@@ -268,16 +296,82 @@ class Marker { | |
this._lngLat = smartWrap(this._lngLat, this._pos, this._map.transform); | ||
} | ||
|
||
this._pos = this._map.project(this._lngLat)._add(this._offset); | ||
const pos = this._pos = this._map.project(this._lngLat); | ||
|
||
let anchor = this.options.anchor; | ||
const offset = normalizeOffset(this.options.offset); | ||
|
||
if (!anchor) { | ||
const width = this._element.offsetWidth, | ||
height = this._element.offsetHeight; | ||
|
||
if (pos.y + offset.bottom.y < height) { | ||
anchor = ['top']; | ||
} else if (pos.y > this._map.transform.height - height) { | ||
anchor = ['bottom']; | ||
} else { | ||
anchor = []; | ||
} | ||
|
||
if (pos.x < width / 2) { | ||
anchor.push('left'); | ||
} else if (pos.x > this._map.transform.width - width / 2) { | ||
anchor.push('right'); | ||
} | ||
|
||
if (anchor.length === 0) { | ||
anchor = 'bottom'; | ||
} else { | ||
anchor = anchor.join('-'); | ||
} | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similar to the discussion above re: offset, I'd be in favor of keeping the default behavior simpler, i.e., simply defaulting to |
||
|
||
const offsetedPos = pos.add(offset[anchor]); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. for some reason, this part right here seems to be breaking all my tests, usually in the form of add |
||
|
||
// because rounding the coordinates at every `move` event causes stuttered zooming | ||
// we only round them when _update is called with `moveend` or when its called with | ||
// no arguments (when the Marker is initialized or Marker#setLngLat is invoked). | ||
if (!e || e.type === "moveend") { | ||
this._pos = this._pos.round(); | ||
this._pos = offsetedPos.round(); | ||
} | ||
|
||
const anchorTranslate = { | ||
'top': 'translate(-50%,0)', | ||
'top-left': 'translate(0,0)', | ||
'top-right': 'translate(-100%,0)', | ||
'bottom': 'translate(-50%,-100%)', | ||
'bottom-left': 'translate(0,-100%)', | ||
'bottom-right': 'translate(-100%,-100%)', | ||
'left': 'translate(0,-50%)', | ||
'right': 'translate(-100%,-50%)' | ||
}; | ||
|
||
const classList = this._element.classList; | ||
for (const key in anchorTranslate) { | ||
classList.remove(`mapboxgl-marker-anchor-${key}`); | ||
} | ||
classList.add(`mapboxgl-marker-anchor-${anchor}`); | ||
|
||
DOM.setTransform(this._element, `translate(-50%, -50%) translate(${this._pos.x}px, ${this._pos.y}px)`); | ||
DOM.setTransform(this._element, `${anchorTranslate[anchor]} translate(${offsetedPos.x}px,${offsetedPos.y}px)`); | ||
} | ||
|
||
/** | ||
* Get the marker's anchor. | ||
* @returns {Point} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. At the top you have set the Anchor type as There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. gotcha, thank you for going through this! I might have missed this while resetting git to avoid the conflicts. |
||
*/ | ||
getAnchor() { | ||
return this._anchor; | ||
} | ||
|
||
/** | ||
* Sets the anchor of the marker | ||
* @param {PointLike} [anchor] The anchor in pixels as a {@link PointLike} object to apply relative to the element's center. Negatives indicate left and up. | ||
* @returns {Marker} `this` | ||
*/ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. is the PointLike an issue here? this smells kind of funny, unless this is interpolating the anchor There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. if we refer back to the original ticket #5640, the anchor option should be a string like bottom, top-left, etc. A PointLike is an x,y pixel value. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ahhh I see, thank you |
||
setAnchor(anchor: PointLike) { | ||
this._anchor = Point.convert(anchor); | ||
this._update(); | ||
return this; | ||
} | ||
|
||
/** | ||
|
@@ -300,4 +394,51 @@ class Marker { | |
} | ||
} | ||
|
||
function normalizeOffset(offset: ?Offset) { | ||
if (!offset) { | ||
return normalizeOffset(new Point(0, 0)); | ||
|
||
} else if (typeof offset === 'number') { | ||
// input specifies a radius from which to calculate offsets at all positions | ||
const cornerOffset = Math.round(Math.sqrt(0.5 * Math.pow(offset, 2))); | ||
return { | ||
'top': new Point(0, offset), | ||
'top-left': new Point(cornerOffset, cornerOffset), | ||
'top-right': new Point(-cornerOffset, cornerOffset), | ||
'bottom': new Point(0, -offset), | ||
'bottom-left': new Point(cornerOffset, -cornerOffset), | ||
'bottom-right': new Point(-cornerOffset, -cornerOffset), | ||
'left': new Point(offset, 0), | ||
'right': new Point(-offset, 0) | ||
}; | ||
|
||
} else if (offset instanceof Point || Array.isArray(offset)) { | ||
// input specifies a single offset to be applied to all positions | ||
const convertedOffset = Point.convert(offset); | ||
return { | ||
'top': convertedOffset, | ||
'top-left': convertedOffset, | ||
'top-right': convertedOffset, | ||
'bottom': convertedOffset, | ||
'bottom-left': convertedOffset, | ||
'bottom-right': convertedOffset, | ||
'left': convertedOffset, | ||
'right': convertedOffset | ||
}; | ||
|
||
} else { | ||
// input specifies an offset per position | ||
return { | ||
'top': Point.convert(offset['top'] || [0, 0]), | ||
'top-left': Point.convert(offset['top-left'] || [0, 0]), | ||
'top-right': Point.convert(offset['top-right'] || [0, 0]), | ||
'bottom': Point.convert(offset['bottom'] || [0, 0]), | ||
'bottom-left': Point.convert(offset['bottom-left'] || [0, 0]), | ||
'bottom-right': Point.convert(offset['bottom-right'] || [0, 0]), | ||
'left': Point.convert(offset['left'] || [0, 0]), | ||
'right': Point.convert(offset['right'] || [0, 0]) | ||
}; | ||
} | ||
} | ||
|
||
module.exports = Marker; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also needs
center
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, yeah I think @andrewharvey 's right, it should be
middle
, notcenter