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

Refactor /services loadTiles #5104

Closed
thomas-hervey opened this issue Jun 22, 2018 · 4 comments
Closed

Refactor /services loadTiles #5104

thomas-hervey opened this issue Jun 22, 2018 · 4 comments
Assignees
Labels
chore Improvements to the iD development experience or codebase

Comments

@thomas-hervey
Copy link
Collaborator

Several services, including the image viewers like openstreetcam, mapillary, and streetside (along with notes and osm) require loading tiles. Besides checking for nullIsland (as the image viewer ones do), all of these files load tiles in a very similar way. See openstreetcam for example.

This could probably be refactored to a utility.

@thomas-hervey thomas-hervey added the chore Improvements to the iD development experience or codebase label Jun 22, 2018
@thomas-hervey thomas-hervey self-assigned this Jun 22, 2018
@bhousel
Copy link
Member

bhousel commented Jun 22, 2018

Yes! Strong 👍 on this one..
It's been something I've wanted to do for a while.

We do kind of have a utility already in d3.geo.tile.js, so it might make sense to:

  1. move a lot of the common code into there
  2. while also adding the capability to use variable tile sizes (it's currently hardcoded to 256px)

For historical context, d3.geo.tile is an older library that we pulled into iD and changed a little bit. The version we use works fine for our use. An old version of it lives here and hasn't been updated in years. That d3 plugins repo is not maintained anymore.

I think the best way forward might be to move our internal lib/d3.geo.tile.js module into a /util/utilTile.js module with a new name so it wouldn't be confused with the old D3 v3 plugin, then give it the superpowers listed above.

@thomas-hervey
Copy link
Collaborator Author

@bhousel I once I have a stable working notes implementation, I can try to tackle this.

@thomas-hervey
Copy link
Collaborator Author

Several changes made in master after this merge including e4d829e d1fe81b ff64455 02713e4

@bhousel
Copy link
Member

bhousel commented Jul 21, 2018

I'm still working on a few more cleanups, and supporting sizes other than 256px.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
chore Improvements to the iD development experience or codebase
Projects
None yet
Development

No branches or pull requests

2 participants