Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Add bearing to markers #8836
Add bearing to markers #8836
Changes from 13 commits
cfddbaa
921304c
b7393e2
d761463
31527d8
41a966c
f770abf
1ec0f83
1213a6d
7a49c34
633e5eb
9053c48
9aa7609
bc32342
e843f5d
6b3b273
07b117d
092a5d7
f38ba7b
0250512
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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 far as I can tell, nothing in this PR restricts rotation to the clockwise direction (which I think is good!). If you set
rotation = -45
, then the marker will rotate 45 degrees counterclockwise, right? So we should spell that out here.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.
Maybe just enough to replace "clockwise, in degrees" with "a positive rotation angle is clockwise, units in degrees"?
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.
Maybe
The rotation angle of the marker in degrees, relative to its respective {@link Marker#rotationAlignment} setting. A positive value will rotate the marker clockwise.
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.
Updated the parameter description.
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 explicitly note that by default,
pitchAlignment
androtationAlignment
are set toauto
which in the case ofrotationAlignment
is equivalent toviewport
. But by analogy withicon-rotation-alignment
andicon-pitch-alignment
, we can see that ificon-pitch-alignment
is set toauto
, it matches whatevericon-rotation-alignment
is set to. I think it would be good to match that behavior with these 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.
I also think that we could more closely align the wording of the descriptions here with what we have already for the symbol style properties. So for
pitchAlignment
, something like "map
aligns theMarker
to the plane of the map.viewport
aligns theMarker
to the plane of the viewport`." We also should use proper punctuation in the comments.cc @chloekraw for thoughts on wording (you may want to review the wording for the function descriptions starting on line 515 as well)
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 used your wording for
pitchAlignment
, but there are extra properties that make theicon-rotation-alignment
description as easy to drop in. Changed that to:"
map
aligns theMarker
's rotation relative to the map, maintaining a bearing as the map rotates.viewport
aligns theMarker
's rotation relative to the viewport, agnostic to map rotations.auto
is equivalent toviewport
."Thoughts?
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.
Again, I think if
options.pitchAlignment
isauto
,this._pitchAlignment
should match whateveroptions.rotationAlignment
is.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.
Set to
options && options.pitchAlignment || this._rotationAlignment
, putting it afterthis._rotationAlignment
is set.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 the wording on this comment isn't quite right. If the marker is set relative to the viewport, then it's not really correct to say it's pointing in a particular "direction". There's also no indication in the description of the method or the
rotation
parameter thatrotation
is in degrees or what positive and negative degrees would do (clockwise vs counter-clockwise).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.
Agreed this should be changed, but it goes back to if we should repeat the documentation for the property, or omit duplicating it and expect people to refer back to the
rotation
property documentation.I think less duplication is better as you don't need to re-read the same documentation in multiple places and ensure it's consistent.
* @param {number} [options.rotation=0] The rotation angle of the marker (clockwise, in degrees), relative to its respective {@link Marker#rotationAlignment} setting.
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.
Updated to "Sets the
rotationAlignment
of the 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.
in degrees
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.
Fixed.
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.
reference to direction here as well. I'm not sure what the best way to say this is, but I think this isn't quite right. @andrewharvey thoughts?
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 would just use "Sets the
rotationAlignment
of the marker." Further documentation of exactly what rotationAlignment is is further up in the options and I don't think needs to be duplicated here? At least this is what other options like draggable/setDraggable do. Open to other opinions though.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 like that approach and I totally missed that this needs to be
rotationAlignment
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.
Used that wording for
setPitchAlignment
andsetRotationAlignment
, and fixed therotateAlignment
typo.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 would also need to change to match
rotationAlignment
whenalignment
isauto
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.
Updated to
alignment || this._rotationAlignment
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 probably add a test that ensures setting
rotationAlignment === 'map'
andpitchAlignment === 'auto'
results inpitchAlignment === 'map'
. Both through the options object and by callingset{Rotation/Pitch}Alignment
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.
Added a couple tests. One is for the option on initialization, the other is for the getter/setters.