-
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
Conversation
added anchor option for marker
Revert "added anchor option for marker"
src/ui/marker.js
Outdated
* @example | ||
* var markerHeight = 50, markerRadius = 10, linearOffset = 25; | ||
* var markerOffsets = { |
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.
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 comment
The 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 comment
The 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 comment
The 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 comment
The reason will be displayed to describe this comment to others. Learn more.
As @andrewharvey noted above, the reason we need this in Popup
is that it dynamically decides the anchor based on the popup's position relative to the edges of the viewport. I don't think Marker
should include such logic -- it's lower-level than Popup
, and the use case for such automatic anchoring magic seems less clear.
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.
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
src/ui/marker.js
Outdated
@@ -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 comment
The reason will be displayed to describe this comment to others. Learn more.
middle
should be an option, and it should be the default option (it what happens currently in master)
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.
Thanks very much for this contribution, @jay-manday! This is off to a good start, but I'm thinking we should keep the API a bit simpler -- please see my comments inline.
src/ui/marker.js
Outdated
* @example | ||
* var markerHeight = 50, markerRadius = 10, linearOffset = 25; | ||
* var markerOffsets = { |
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.
As @andrewharvey noted above, the reason we need this in Popup
is that it dynamically decides the anchor based on the popup's position relative to the edges of the viewport. I don't think Marker
should include such logic -- it's lower-level than Popup
, and the use case for such automatic anchoring magic seems less clear.
src/ui/marker.js
Outdated
@@ -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 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
, not center
src/ui/marker.js
Outdated
} else { | ||
anchor = anchor.join('-'); | ||
} | ||
} |
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.
Similar to the discussion above re: offset, I'd be in favor of keeping the default behavior simpler, i.e., simply defaulting to center
(since that's the current behavior)
src/ui/marker.js
Outdated
* `'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 comment
The reason will be displayed to describe this comment to others. Learn more.
relative to the element's center
This will now be "relative to the anchor"
src/ui/marker.js
Outdated
* `'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 comment
The reason will be displayed to describe this comment to others. Learn more.
"relative to the element's center"
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 comment
The 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 comment
The 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 :)
I've made the requested changes to marker, though still getting stumped with the proper testing procedure (breaking less though!) to mock the new anchor option. This has been a good learning experience for me, I don't have too much experience writing these kinds of tests though always strive for succinct code. Thank you so so much for your time, patience and mentorship @anandthakker @andrewharvey I super appreciate it. |
src/ui/marker.js
Outdated
* @param {Object} [options] | ||
* @param {string} [options.anchor] - A string indicating the markers's location relative to | ||
* the coordinate set via {@link Marker#setLngLat}. | ||
* Options is `'middle'` by default |
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.
See https://github.com/mapbox/mapbox-gl-js/blob/master/src/ui/map.js#L171 for an example of how to set the default value directly in the JSDoc. The documentation generator automatically picks up this default value as you can see https://www.mapbox.com/mapbox-gl-js/api/#map
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 like
Line 171 in d11cf30
* @param {string} [options.logoPosition='bottom-left'] A string representing the position of the Mapbox wordmark on the map. Valid options are `top-left`,`top-right`, `bottom-left`, `bottom-right`. |
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.
ty!! omg, if this works i'm probably going to cry lol 😂
src/ui/marker.js
Outdated
@@ -11,27 +11,43 @@ import type Popup from './popup'; | |||
import type {LngLatLike} from "../geo/lng_lat"; | |||
import type {MapMouseEvent} from './events'; | |||
|
|||
export type Anchor = 'middle'; |
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.
You had it right the first time enumerating the valid options like https://github.com/mapbox/mapbox-gl-js/blob/master/src/ui/popup.js#L19
@anandthakker any thoughts on if Marker should import the Anchor type from Popup? That would mean adding middle
as a popup anchor option...
@jay-manday Keep it up! I think it might be helpful to re-look at how the API should work. One way to approach it is to write all your unit tests first, as that defines how you expect the API to work, then you can start chipping away at the code to make them pass (test driven development). It looks like you're getting Anchor (a string like 'middle', 'bottom', 'top', etc) and Offset (an x,y pixel offset to move the Marker image relative to the anchor) mixed up in the code, and if we get the test cases right first in means we have defined how the API should work. |
ty soo much @andrewharvey! I made a ton of progress last night, only a handful of tests failing at this point. |
Still working on things and fleshed out the testing for marker. It seems the only major hurdle remaining is testing through the enumerated anchors and offsets, i'm going to keep working on this though of would welcome reviews of my progress. 🐌 Thank you! |
src/ui/marker.js
Outdated
@@ -11,26 +11,53 @@ import type Popup from './popup'; | |||
import type {LngLatLike} from "../geo/lng_lat"; | |||
import type {MapMouseEvent} from './events'; | |||
|
|||
export type Anchor = 'middle' | '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 comment
The reason will be displayed to describe this comment to others. Learn more.
👍 this looks like all the options that we should support
src/ui/marker.js
Outdated
* @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 {Array<string> | string} [options.anchor = Anchor] - A string indicating the markers's location relative to |
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.
See "An optional parameter and default value" at http://usejsdoc.org/tags-param.html the default value should be middle
.
See https://github.com/mapbox/mapbox-gl-js/blob/master/src/ui/popup.js#L37, the options.anchor type is only string
. Each marker can only have one anchor so the type should just be string.
src/ui/marker.js
Outdated
@@ -127,11 +154,31 @@ class Marker { | |||
svg.appendChild(page1); | |||
|
|||
element.appendChild(svg); | |||
|
|||
// if no element and no offset option given apply an offset for the default 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.
best to rebase from master so these changes are brought in that way. that should also resolve the merge conflicts
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.
} | ||
} | ||
|
||
const offsetedPos = pos.add(offset[anchor]); |
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.
for some reason, this part right here seems to be breaking all my tests, usually in the form of add undefined
or expecting a truthy value
test/unit/ui/marker.test.js
Outdated
const marker = new Marker(el); | ||
t.ok(marker.getElement(this.options), 'default marker is created'); | ||
t.ok(marker.getAnchor(), `marker set with anchor to default`); | ||
t.ok(marker.getOffset(), 'default marker with no offset uses default marker offset'); | ||
t.end(); | ||
}); | ||
|
||
t.test('default marker with some options', (t) => { |
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.
let's leave the original test in place, you can write a new test for the functionality you've added.
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.
will do 👍
test/unit/ui/marker.test.js
Outdated
t.end(); | ||
}); | ||
|
||
t.test('default marker with custom offest', (t) => { | ||
const marker = new Marker(null, { offset: [1, 2] }); | ||
const el = window.document.createElement(`mapboxgl-marker`); | ||
const marker = new Marker(el, { offset: [1, 2] }); |
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.
this test was originally written for the default marker, ie. null must be passed as the element, I'd suggest leaving this test in place and writing a new one for anchor
test/unit/ui/marker.test.js
Outdated
.setPopup(popup) | ||
.setLngLat([-77.01866, 38.888]) | ||
.addTo(map); | ||
t.deepEqual(popup.getLngLat(), new LngLat(-77.01866, 38.888)); | ||
t.end(); | ||
}); | ||
|
||
t.test('marker centered by default', (t) => { |
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.
we still need to test that the marker is centered by default
@@ -109,16 +239,5 @@ test('Marker', (t) => { | |||
t.end(); | |||
}); | |||
|
|||
t.test('marker\'s offset can be changed', (t) => { |
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.
we still need to test that the marker's offset can be changed
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.
Gotcha, thank you for going through this! I'm still getting used to working with flow and will be making the necessary changes and pushing shortly.
* 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 comment
The 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 comment
The 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 comment
The reason will be displayed to describe this comment to others. Learn more.
ahhh I see, thank you
Sorry for the delay @andrewharvey @anandthakker, things got a little hectic at the start of the semester and then got hit hard with the flu 😷 feeling better though! Thank you both for the hand holding on this one, i've really learnt a ton going through this process. I went through this and things should be working properly now! When you get a chance please take a look and let me know about any further tweaks and changes 👍 |
test/unit/ui/marker.test.js
Outdated
t.ok(marker.getElement(), 'default marker is created'); | ||
t.ok(marker.getOffset().equals(new Point(0, -14)), 'default marker with no offset uses default marker offset'); | ||
t.ok(marker.getOffset(), 'default marker with no anchor and offset options, uses default marker options'); |
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.
We should leave in a test case for the default marker, ie. no element passed to new Marker()
.
test/unit/ui/marker.test.js
Outdated
t.ok(marker.getElement(), 'default marker is created'); | ||
t.ok(marker.getOffset().equals(new Point(0, -14)), 'default marker with no offset uses default marker offset'); | ||
t.ok(marker.getOffset(), 'default marker with no anchor and offset options, uses default marker options'); | ||
t.end(); | ||
}); | ||
|
||
t.test('default marker with some options', (t) => { |
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.
in this case, "default marker" is when no element is passed to Marker
. We should leave in those unit tests.
test/unit/ui/marker.test.js
Outdated
t.end(); | ||
}); | ||
|
||
t.test('default marker with custom offest', (t) => { | ||
const marker = new Marker(null, { offset: [1, 2] }); | ||
const el = window.document.createElement(`div`); | ||
const marker = new Marker(el, { offset: [1, 2] }); |
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.
same as above, this unit test case is for the "default marker", ie. no element passed to Marker
test/unit/ui/marker.test.js
Outdated
t.end(); | ||
}); | ||
|
||
t.test('marker is added to map', (t) => { | ||
const map = createMap(); | ||
const marker = new Marker(window.document.createElement('div')).setLngLat([-77.01866, 38.888]); | ||
const el = window.document.createElement(`div`); |
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.
I think we should leave in the existing test as is, and add a new unit test(s) with the anchor option. this helps to ensure backwards compatibility is retained and not broken with this PR.
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.
Gotcha, I added the mentioned tests to my latest push and reverted to the window.document.createElement('div')
pattern for the rest of the tests
test/unit/ui/marker.test.js
Outdated
const marker = new Marker(el); | ||
t.ok(marker.getElement(), 'marker element is created'); | ||
const marker = new Marker(); | ||
t.ok(marker, 'marker is created'); |
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.
the original test case here isn't affected by the introduction of the anchor option so best to leave it as is and add new tests if you have new functionality to test.
const marker = new Marker(); | ||
t.ok(marker.getElement(), 'default marker is created'); | ||
t.ok(marker.getOffset().equals(new Point(0, -14)), 'default marker with no offset uses default marker offset'); |
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.
we should still test that the offset and anchor of the default marker is as expected
test/unit/ui/marker.test.js
Outdated
t.end(); | ||
}); | ||
|
||
t.test('default marker with element', (t) => { |
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.
It's not as clear as it could be, but I used the term "default marker" to mean when you create a marker without passing an element, so it uses the default Marker SVG element instead. So "default marker with element" doesn't make sense in that context.
What do you mean by default marker with element?
test/unit/ui/marker.test.js
Outdated
t.ok(marker.getElement(), 'default marker is created'); | ||
t.ok(marker.getOffset().equals(new Point(0, -14)), 'default marker with no offset uses default marker offset'); | ||
t.ok(marker.getAnchor(), 'marker set with anchor options'); |
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.
this only tests that some anchor is returned by getAnchor, we should test that the anchor returned by getAnchor is the same anchor that was set in the initialisation options ie. bottom
.
test/unit/ui/marker.test.js
Outdated
t.end(); | ||
}); | ||
|
||
t.test('default marker with custom offest', (t) => { | ||
const marker = new Marker(null, { offset: [1, 2] }); | ||
const marker = new Marker(window.document.createElement('div'), { offset: [1, 2] }); |
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.
default marker in this test means when no element is passed, let's leave the original test in place and you can add new tests for the new anchor functionality.
test/unit/ui/marker.test.js
Outdated
.setLngLat([0, 0]) | ||
.addTo(map); | ||
|
||
t.ok(marker.setAnchor('middle')); |
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.
Did you mean to check in this test that getAnchor is set to middle by default?
hi @jay-manday the "automatic anchoring magic" (see #6031 (comment)) is still in this PR. If you strongly feel that it's needed, did you want to put together a use case demo? It seems like a very uncommon use case and we're not sure if it justifies the increased API complexity. |
Yes I agree, looking back on things and the prior conversations, allowing users to define a marker offset would complicate things. The edge cases for static maps are really minor compared with how Marker is usually used. Also please pardon my over enthusiastic language describing the proposed offset behavior 😅 I'll go ahead and remove that functionality, thanks again for your patience with me on this PR. I've really learnt a ton. This was my first time writing unit tests and have grown to appreciate the rigor of this work flow. Looking forward to getting this merged (as per approval) so we can move onto fixing other issues : ~ ) |
hi @jay-manday I think there may be a bit of misunderstanding of what the original ticket #5640 is for. We still want to support Marker offset, for example so Markers like this https://github.com/mapbox/makiwich/blob/master/sources/marker-large.svg can be anchored at the tip of the blue dot. |
i'm sorry @andrewharvey, i'm feeling properly silly now. I've reverted to the previous commit. I think the confusion came from mis-interpreting the previous message. I've just realized that I was presenting a case to have offset that was depending on the anchor instead of acting independently as it should |
Launch Checklist
I've been working on adding support for anchor options to the marker element as requested in #5640 and #4751. It's a work in progress and borrows a bit from the
popup
where I thought appropriate. I did some testing and all the cosmetics are clean though getting a little stumped when it hits the tests for marker.js.would love some feedback on my progress so far though! n_n