Skip to content
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

5587 - make geolocation show geolocation #5629

Merged
merged 7 commits into from
Dec 19, 2018
Merged

5587 - make geolocation show geolocation #5629

merged 7 commits into from
Dec 19, 2018

Conversation

maxgrossman
Copy link
Collaborator

the 'puck' radius is geolocation response's accuracy (in meters)

closes #5587

@bhousel
Copy link
Member

bhousel commented Dec 19, 2018

looks good! Just reorder the layers and I can merge 👍

.remove();

var layerEnter = layer.enter()
.append('g')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than just appending it here, I think it might make more sense to insert it before the touch layer, so that clicks on osm objects will still work. something like: insert('g', '.data-layer.touch')

Forget what I said here.. If layer-geolocate is before layer-touch you can append the circle to it.

import _throttle from 'lodash-es/throttle';

export function svgGeolocate(projection, context, dispatch) {
var throttledRedraw = _throttle(function () { dispatch.call('change'); }, 1000);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not 100% sure but I don't think this code should ever need to dispatch change.. This forces everything to redraw. Maybe this code could maintain its own timer to refresh the location circle sometimes.

@@ -32,7 +33,8 @@ export function svgLayers(projection, context) {
{ id: 'mapillary-signs', layer: svgMapillarySigns(projection, context, dispatch) },
{ id: 'openstreetcam-images', layer: svgOpenstreetcamImages(projection, context, dispatch) },
{ id: 'debug', layer: svgDebug(projection, context, dispatch) },
{ id: 'touch', layer: svgTouch(projection, context, dispatch) }
{ id: 'touch', layer: svgTouch(projection, context, dispatch) },
{ id: 'geolocate', layer: svgGeolocate(projection, context, dispatch) }
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh cool I see now.. Yeah geolocate should be before touch I think, so it doesn't steal the user's mouse clicks.

@@ -36,6 +36,7 @@ describe('iD.svgLayers', function () {
expect(d3.select(nodes[6]).classed('openstreetcam-images')).to.be.true;
expect(d3.select(nodes[7]).classed('debug')).to.be.true;
expect(d3.select(nodes[8]).classed('touch')).to.be.true;
expect(d3.select(nodes[9]).classed('geolocate')).to.be.true;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

here too

@maxgrossman maxgrossman changed the title 5587 - make geolocation show geolocatoni 5587 - make geolocation show geolocation Dec 19, 2018
@bhousel bhousel merged commit 81b5fc7 into master Dec 19, 2018
@bhousel bhousel deleted the 5587 branch December 19, 2018 17:14
@bhousel
Copy link
Member

bhousel commented Dec 19, 2018

Thanks @maxgrossman 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make geolocate button show the location
2 participants