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

New providers added #267

Merged
merged 9 commits into from
Apr 11, 2018
Merged

New providers added #267

merged 9 commits into from
Apr 11, 2018

Conversation

chatelao
Copy link
Contributor

@chatelao chatelao commented Apr 7, 2018

OpenFireMap, OpenInfraMap

@jieter
Copy link
Contributor

jieter commented Apr 9, 2018

@chatelao thanks for your PR. Can you pllease have a look at why the tests fail, and fix those issues?

@chatelao
Copy link
Contributor Author

chatelao commented Apr 9, 2018

Fix applied (formating error)

OpenSeaMap: {
url: 'https://tiles.openseamap.org/seamark/{z}/{x}/{y}.png',
options: {
attribution: 'Map data: &copy; <a href="http://www.openseamap.org">OpenSeaMap</a> contributors'
}
},
OpenPtMap: {
maxZoom: 17,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this one has to go into options?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will have a look. Something seamed wierd with the zoom levela.

Copy link
Contributor

Choose a reason for hiding this comment

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

The top level object should only have keys url, options and variants, so the maxZoom should not be in L.TileLayer.Provider.providers.OpenPtMap but in L.TileLayer.Provider.providers.OpenPtMap.options.

@chatelao
Copy link
Contributor Author

You are so nice and helpful. I will commit after fixing. Maybe you tests support checking a json schema? I could try to create one.

@chatelao
Copy link
Contributor Author

Next try :-)

@jieter
Copy link
Contributor

jieter commented Apr 11, 2018

Your commit 4a3a8d2 was fine (Tests passed), please remove the last commit (28aa79f).

@chatelao
Copy link
Contributor Author

Thank you for your pation with a newbie.

@jieter
Copy link
Contributor

jieter commented Apr 11, 2018

I added a bit more strictness to the tests, which can be seen here.

This looks good to me, I'm merging.

For next time, it's helpful to include all layer's you are adding with the PR to the description of the PR and if possible include links to their usage terms.

@jieter jieter merged commit 30233ec into leaflet-extras:master Apr 11, 2018
@jieter
Copy link
Contributor

jieter commented Apr 11, 2018

@chatelao if I may suggest some improvements:

  • OpenStreetMap.CH could use bounds, as it is really only defined for Switzerland.
  • OpenInfraMap.* are overlays, so it would be nice if the preview would render them on a background layer. This would mean adding a regexp to isOverlay
  • Same is true for OpenRailwayMap, OpenFireMap and SafeCast.

@chatelao
Copy link
Contributor Author

Okay, I will try this. Didn't see this part of the code (yet).

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.

2 participants