default marker offset

[andrewharvey]

Launch Checklist

• briefly describe the changes in this PR
  Provide default element for Marker class per #3557 #5661 implemented a default Marker element, this PR applies an offset for that default Marker, so the tip of the marker image is located at the marker lng/lat.

• write tests for all new functionality

• document any changes to public APIs
  I don't think it needs any special documentation as I'd consider it to be expected behaviour so I consider this to be a bug fix rather than a breaking change.

• post benchmark scores

• manually test the debug page

[mollymerp]

This change makes sense to me and looks good when testing locally – I just had one comment about the offset amount.

[mollymerp]

did you determine the -14 through trial and error or some other way? seeing that the SVG is 41px tall makes this number non-obvious to me, although it looks great when I tested the branch. it would be nice to avoid hardcoding this constant and get the offset from some of the SVG properties.

[andrewharvey]

I actually opened the SVG in Inkscape and measured the distance from the image center to where the tip is, but you could calculate it based on the center of the shadow ellipse, from the SVG, but it would add a fair bit more code and not sure how useful that would be. I've added some comments on where the -14 magic number came from.

What do you think, should be leave it as is with the comment or go ahead and calculate it on the fly from the SVG?

[mollymerp]

gotcha – I think we can leave it hardcoded for now with the comment.

[mollymerp]

Thanks @andrewharvey – this works for me. One thing I thought of was maybe adding a comment to the element param of Marker constructor to indicate that, if an element is provided, the marker will be placed such that the center of the element is on the LngLat they set so users will know they need to offset if they want different behavior.

[andrewharvey]

Thanks @andrewharvey – this works for me. One thing I thought of was maybe adding a comment to the element param of Marker constructor to indicate that, if an element is provided, the marker will be placed such that the center of the element is on the LngLat they set so users will know they need to offset if they want different behavior.

Thanks for reviewing @mollymerp .

@jay-manday is working on Marker anchor at #6031 which also brings in Flow and better JSDoc for Marker. I think once the anchor option is introduced it makes it a bit clearer that by default they are anchored to the middle, so I'm tempted to leave the comment out and have it addressed in #6031, which also helps reduce the conflicts which have arisen between both PRs.

Does that work?

[anandthakker]

Thanks @andrewharvey !