WIP: added support for anchor option on Marker #6031

jmandel1027 · 2018-01-21T18:36:28Z

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

briefly describe the changes in this PR
write tests for all new functionality
document any changes to public APIs
post benchmark scores
manually test the debug page

added anchor option for marker

Revert "added anchor option for marker"

andrewharvey · 2018-01-21T22:36:16Z

src/ui/marker.js

 * @example
+ * var markerHeight = 50, markerRadius = 10, linearOffset = 25;
+ * var markerOffsets = {


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?

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?

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.

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 ✨🎷🐛

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.

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

andrewharvey · 2018-01-25T02:22:00Z

src/ui/marker.js

@@ -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';


middle should be an option, and it should be the default option (it what happens currently in master)

anandthakker

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.

anandthakker · 2018-01-25T02:06:37Z

src/ui/marker.js

 * @example
+ * var markerHeight = 50, markerRadius = 10, linearOffset = 25;
+ * var markerOffsets = {


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.

anandthakker · 2018-01-25T02:14:44Z

src/ui/marker.js

@@ -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';


Also needs center

Ah, yeah I think @andrewharvey 's right, it should be middle, not center

anandthakker · 2018-01-25T02:15:55Z

src/ui/marker.js

+            } else {
+                anchor = anchor.join('-');
+            }
+        }


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)

anandthakker · 2018-01-25T02:19:40Z

src/ui/marker.js

+ *   `'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.


relative to the element's center

This will now be "relative to the anchor"

andrewharvey · 2018-01-25T02:23:44Z

src/ui/marker.js

+ *   `'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.


"relative to the element's center"

I think with the anchor option it's clearer to say relative to the element's anchor.

Whops, sorry didn't see @anandthakker's comment

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 :)

jmandel1027 · 2018-01-25T04:12:03Z

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.

andrewharvey · 2018-01-25T04:17:11Z

src/ui/marker.js

+ * @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


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

Also like

mapbox-gl-js/src/ui/map.js

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`.

I'd include a list of all the valid options for 'anchor'

ty!! omg, if this works i'm probably going to cry lol 😂

andrewharvey · 2018-01-25T04:21:07Z

src/ui/marker.js

@@ -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';


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...

andrewharvey · 2018-01-25T04:27:46Z

@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.

jmandel1027 · 2018-01-25T14:15:13Z

ty soo much @andrewharvey!

I made a ton of progress last night, only a handful of tests failing at this point.
When i've got all the tests cleared I'll push my changes 🌮 🎉

jmandel1027 · 2018-01-25T22:34:16Z

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!

andrewharvey · 2018-01-25T22:51:33Z

src/ui/marker.js

@@ -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';


👍 this looks like all the options that we should support

andrewharvey · 2018-01-25T23:19:15Z

src/ui/marker.js

- * @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


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.

andrewharvey · 2018-01-25T23:26:13Z

src/ui/marker.js

@@ -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


best to rebase from master so these changes are brought in that way. that should also resolve the merge conflicts

sure thing, and apologies for consuming so much time with this, i feel like i've learned a lot going through this process though 🌀 👍

on a lighter note, i've resolved those conflicts!

jmandel1027 · 2018-01-25T23:32:06Z

src/ui/marker.js

+            }
+        }
+
+        const offsetedPos = pos.add(offset[anchor]);


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

andrewharvey · 2018-01-26T22:19:06Z

test/unit/ui/marker.test.js

+        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) => {


let's leave the original test in place, you can write a new test for the functionality you've added.

will do 👍

andrewharvey · 2018-01-26T22:20:15Z

test/unit/ui/marker.test.js

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


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

andrewharvey · 2018-01-26T22:23:05Z

test/unit/ui/marker.test.js

            .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) => {


we still need to test that the marker is centered by default

andrewharvey · 2018-01-26T22:23:34Z

test/unit/ui/marker.test.js

@@ -109,16 +239,5 @@ test('Marker', (t) => {
        t.end();
    });

-    t.test('marker\'s offset can be changed', (t) => {


we still need to test that the marker's offset can be changed

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.

jmandel1027 · 2018-01-30T04:41:20Z

src/ui/marker.js

+     * 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`
+     */


is the PointLike an issue here? this smells kind of funny, unless this is interpolating the anchor

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.

ahhh I see, thank you

jmandel1027 · 2018-02-05T02:08:50Z

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 👍

andrewharvey · 2018-02-14T07:46:50Z

test/unit/ui/marker.test.js

        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');


We should leave in a test case for the default marker, ie. no element passed to new Marker().

andrewharvey · 2018-02-14T07:48:06Z

test/unit/ui/marker.test.js

        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) => {


in this case, "default marker" is when no element is passed to Marker. We should leave in those unit tests.

andrewharvey · 2018-02-14T07:48:45Z

test/unit/ui/marker.test.js

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


same as above, this unit test case is for the "default marker", ie. no element passed to Marker

andrewharvey · 2018-02-14T07:50:03Z

test/unit/ui/marker.test.js

        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`);


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.

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

andrewharvey · 2018-02-15T04:04:10Z

test/unit/ui/marker.test.js

-        const marker = new Marker(el);
-        t.ok(marker.getElement(), 'marker element is created');
+        const marker = new Marker();
+        t.ok(marker, 'marker is created');


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.

andrewharvey · 2018-02-15T04:05:21Z

test/unit/ui/marker.test.js

        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');


we should still test that the offset and anchor of the default marker is as expected

andrewharvey · 2018-02-15T04:07:54Z

test/unit/ui/marker.test.js

+        t.end();
+    });
+
+    t.test('default marker with element', (t) => {


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?

andrewharvey · 2018-02-15T04:09:17Z

test/unit/ui/marker.test.js

        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');


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.

andrewharvey · 2018-02-15T04:10:25Z

test/unit/ui/marker.test.js

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


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.

andrewharvey · 2018-02-15T04:12:19Z

test/unit/ui/marker.test.js

+            .setLngLat([0, 0])
+            .addTo(map);
+
+        t.ok(marker.setAnchor('middle'));


Did you mean to check in this test that getAnchor is set to middle by default?

andrewharvey · 2018-02-15T04:15:14Z

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.

jmandel1027 · 2018-02-15T10:47:52Z

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 : ~ )

andrewharvey · 2018-02-16T02:04:07Z

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.

jmandel1027 · 2018-02-16T06:18:44Z

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

jmandel1027 added 9 commits January 15, 2018 23:31

added mouseover event

19d42db

testing mouseover event handler

5ad5789

added anchor option for marker

47d1243

Merge pull request #2 from jay-manday/marker-anchor

3da1c0d

added anchor option for marker

Revert "added anchor option for marker"

ada8e92

Merge pull request #3 from jay-manday/revert-2-marker-anchor

a9791fc

Revert "added anchor option for marker"

merging upstream

2270123

added support for anchor option on marker

e0972bb

progress on adding anchor support to marker WIP

dcfc710

jmandel1027 changed the title ~~added support for anchor option on Marker (WIP)~~ WIP: added support for anchor option on Marker Jan 21, 2018

andrewharvey reviewed Jan 21, 2018

View reviewed changes

andrewharvey mentioned this pull request Jan 25, 2018

default marker offset #6012

Merged

5 tasks

andrewharvey reviewed Jan 25, 2018

View reviewed changes

anandthakker suggested changes Jan 25, 2018

View reviewed changes

andrewharvey reviewed Jan 25, 2018

View reviewed changes

made requested tweaks to marker, though still stumping tests

532b453

andrewharvey reviewed Jan 25, 2018

View reviewed changes

andrewharvey added the under development label Jan 25, 2018

progress though stuck on testing through enumerated anchors + offsets

c4e2aca

andrewharvey reviewed Jan 25, 2018

View reviewed changes

jmandel1027 commented Jan 25, 2018

View reviewed changes

jmandel1027 added 2 commits January 25, 2018 20:58

Merge branch 'master' into master

278ed67

completed testing for marker anchor and offset options

c3b9400

andrewharvey reviewed Jan 26, 2018

View reviewed changes

jmandel1027 commented Jan 30, 2018

View reviewed changes

wrote further tests for new marker behaviors

7211a29

jmandel1027 added 2 commits February 4, 2018 21:12

minor fix on marker

c938fbd

wrote additional test for marker anchor behaviors

9996b21

jmandel1027 mentioned this pull request Feb 14, 2018

change "drag a point" example to use Marker #6102

Closed

andrewharvey reviewed Feb 14, 2018

View reviewed changes

wrote more tests for marker behaviors

df4308a

andrewharvey reviewed Feb 15, 2018

View reviewed changes

jmandel1027 force-pushed the master branch from 02d6143 to df4308a Compare February 16, 2018 06:22

jmandel1027 added 3 commits February 16, 2018 03:07

revised tests

fae2681

revising tests

c8d0bd1

revised tests

4275456

jmandel1027 closed this Feb 21, 2018

WIP: added support for anchor option on Marker #6031

WIP: added support for anchor option on Marker #6031

Conversation

jmandel1027 commented Jan 21, 2018

Launch Checklist

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmandel1027 Jan 23, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anandthakker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmandel1027 Jan 25, 2018 • edited Loading

Choose a reason for hiding this comment

jmandel1027 commented Jan 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmandel1027 Jan 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewharvey commented Jan 25, 2018 • edited Loading

jmandel1027 commented Jan 25, 2018 • edited Loading

jmandel1027 commented Jan 25, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmandel1027 Jan 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmandel1027 Jan 30, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jmandel1027 Jan 30, 2018 • edited Loading

Choose a reason for hiding this comment

jmandel1027 commented Feb 5, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andrewharvey commented Feb 15, 2018

jmandel1027 commented Feb 15, 2018 • edited Loading

andrewharvey commented Feb 16, 2018

jmandel1027 commented Feb 16, 2018 • edited Loading

jmandel1027 Jan 23, 2018 •

edited

Loading

jmandel1027 Jan 25, 2018 •

edited

Loading

jmandel1027 commented Jan 25, 2018 •

edited

Loading

jmandel1027 Jan 25, 2018 •

edited

Loading

andrewharvey commented Jan 25, 2018 •

edited

Loading

jmandel1027 commented Jan 25, 2018 •

edited

Loading

jmandel1027 Jan 25, 2018 •

edited

Loading

jmandel1027 Jan 30, 2018 •

edited

Loading

jmandel1027 Jan 30, 2018 •

edited

Loading

jmandel1027 commented Feb 5, 2018 •

edited

Loading

jmandel1027 commented Feb 15, 2018 •

edited

Loading

jmandel1027 commented Feb 16, 2018 •

edited

Loading