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 circle to GeolocateControl showing accuracy of position #9181
add circle to GeolocateControl showing accuracy of position #9181
Changes from 10 commits
8d05a1d
d93e091
d694f36
b290bce
d13e371
036c2e0
f77d16d
e1980e3
b2c4b04
a5ab875
85a11c8
d915500
bbcfbe5
954bb99
ff03457
51d75fc
2af4c8b
87f3393
a13efc0
28603d1
980d1fb
b7e8fb3
8cd474e
8c9ace1
be4f189
99bfc7f
77cfc5c
1eed2ae
fa59cf3
5d8ef1a
03c4339
3574318
1f76684
b3ed503
ae8bd00
d8a5e0d
e2da6ab
0c45a27
5fd231f
412d257
790732a
64e8312
125c623
b1a67c6
6f7f2ef
136a291
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.
Given both markers will always have the same location, do you think it would be better to combine this with the _userLocationDotMarker to have a single Marker? Still having it optional and still using two separate classes for styling.
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.
Personally I think the code will be more complicated trying to force both of these objects to use one Marker: that would introduce some container element that will need to be styled carefully to avoid breaking anything, some additional
if/else
around the flag, and I'd be worried that might introduce some other breaking change related to code I'm not familiar with, whereas I'm pretty confident the current design should be independent. (Obvious caveat I've only been looking at this codebase for 2 days so... let me know ;D)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'd probably prefer a single Marker since their location is always going to be the same, but I can also see how two Markers is easier to implement and in some ways makes the code easier to manage (though single Marker also makes the Marker update code cleaner).
That said, I'm happy with either option.
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 don't think this needs to be addressed within this PR, but eventually we should eliminate the duplication between this and
mapbox-gl-js/src/ui/control/scale_control.js
Lines 124 to 137 in 74b1573
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.
Yeah agreed it's a copy/paste job -- good spot to refactor these into some helpers in the future.
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'll likely forget about it later so let's eliminate duplication now — I think the best place to put it is
LngLat
distanceTo(lngLat)
method. This would follow the same convention as Leaflet.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.
Opened a separate PR for this: #9202