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

move markers to the best world for the current view #4452

Merged
merged 4 commits into from
Mar 22, 2017

Conversation

andrewharvey
Copy link
Collaborator

@andrewharvey andrewharvey commented Mar 20, 2017

Launch Checklist

  • briefly describe the changes in this PR
    DOM Elements like Markers and Popups are only drawn in world 0. However when you're close to the anti-meridian, DOM Elements which should appear might not, and in some cases it makes sense to draw the DOM elements in world -1 and 1.

This aims to address #3770. However until #2071 is fixed, this PR only addresses issues when the view is in world 0.

  • write tests for all new functionality
  • document any changes to public APIs

public LngLat.wrapToWorld(center) added JS doc.

  • post benchmark scores
  • manually test the debug page
    I did some manual testing of this, with both a marker and popup around long 170 and at long -170, and it works well for world -1, 0 and 1, so long as the camera always wraps to world 0 this is enough.

When zoomed out the marker/popup will jump which might be a poor UX, but I think it's okay and probably better than the alternative of it disappearing off the view.

src/util/util.js Outdated
@@ -4,6 +4,7 @@
const UnitBezier = require('@mapbox/unitbezier');
const Coordinate = require('../geo/coordinate');
const Point = require('point-geometry');
const LngLat = require('../geo/lng_lat');
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This creates a circular dependency between util/util.js and geo/lng_lat.js (which uses util.wrap). Which seems to be causing an issue. Investigating...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've removed the circular dependency by moving the bestWorld function out of util/util.

@andrewharvey andrewharvey changed the title move markers to the best world for the current view [wip] move markers to the best world for the current view Mar 20, 2017
@andrewharvey andrewharvey changed the title [wip] move markers to the best world for the current view move markers to the best world for the current view Mar 20, 2017
@mourner
Copy link
Member

mourner commented Mar 20, 2017

Alternatively, instead of a separate util file, we could add a LngLat#snapToWorld(center) method in lnglat.js.

@lucaswoj lucaswoj requested a review from mourner March 20, 2017 20:08
@lucaswoj
Copy link
Contributor

Tagging @mourner for review on this one as he's the most familiar with this part of the code & similar code in Leaflet.

@andrewharvey
Copy link
Collaborator Author

Alternatively, instead of a separate util file, we could add a LngLat#snapToWorld(center) method in lnglat.js.

@mourner Agreed, it feels much cleaner and better style this way. I've made the change and squashed commits.

@andrewharvey
Copy link
Collaborator Author

andrewharvey commented Mar 20, 2017

  • I need to check renderWorldCopies and only enable this behaviour when it is true.

I wanted to add a unit test, but it it didn't seem very easy as I'd need to check the DOM Element CSS transform.

@andrewharvey
Copy link
Collaborator Author

Testing #4452 #4451 #4459 all working together 🎉

out

out2

Copy link
Contributor

@jfirebaugh jfirebaugh left a comment

Choose a reason for hiding this comment

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

Thanks!

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.

None yet

4 participants