Support for Leaflet 1.0.1 and added Typescript-Definition #8
Conversation
ismyrnow
left a comment
ismyrnow
left a comment
There was a problem hiding this comment.
Thanks for the PR getting this working for Leaflet v1!
Ideally, the v1 changes the TypeScript stuff would be separate PRs. The v1 stuff will definitely be merged in, but I expressed some concerns about the TypeScript stuff.
README.md
Outdated
|
|
||
| ## Thanks | ||
|
|
||
| Thanks to @ismyrnow for original version. See [ismyrnow/Leaflet.functionaltilelayer](https://github.com/ismyrnow/Leaflet.functionaltilelayer) |
There was a problem hiding this comment.
We can get away without this. If you want to maintain a fork and have it there, that's fine by me, but it's a little redundant in this repo.
There was a problem hiding this comment.
You're right :) first i wanted to fork only - but then .... will delete this.
| .replace('{2}', view.tile.column) | ||
| .replace('{3}', view.subdomain); | ||
| var url = 'http://{s}.tile.osm.org/{z}/{x}/{y}.png' |
There was a problem hiding this comment.
Thanks for bringing in valid tiles (since the mq tiles went away)!
| Leaflet.functionaltilelayer | ||
| =========================== | ||
|
|
||
| For use with Leaflet 1.0.1. |
There was a problem hiding this comment.
I might tweak this a bit, to note that an older version is available. You can leave it how it is for now.
There was a problem hiding this comment.
I will add a version number 1.0.0 to package.json. Maybe you could create a tag for 0.7.
src/leaflet.functionaltilelayer.js
Outdated
|
|
||
| this._adjustTilePoint(tilePoint); | ||
| var tileUrl = this.getTileUrl(tilePoint); | ||
| L.DomEvent.on(tile, 'load', L.bind(this._tileOnLoad, this, done, tile)); |
There was a problem hiding this comment.
The indentation looks to be incorrect here. Can you adjust it to match the other lines?
package.json
Outdated
| "license": "MIT" | ||
| "license": "MIT", | ||
| "dependencies": { | ||
| "@types/leaflet": "^1.0.34" |
There was a problem hiding this comment.
Is this dependency necessary for people just using the plugin? Shouldn't it be a devDependency?
I want to be sensitive to developers who aren't using TypeScript, which is still the majority of developers. I also don't use TypeScript myself, so I'm not sure what the best route to take is.
There was a problem hiding this comment.
The TypeScript-Handbook suggests to use dependencies: Publishing. But i understand your concerns. I will remove the TypeScript-stuff and maintain this in a fork. I will read about it and maybe create a different pull request.
|
Removed all the TypeScript-stuff and fixed the indentation-errors. Please review. |
ismyrnow
left a comment
ismyrnow
left a comment
There was a problem hiding this comment.
Thanks so much for doing that! This is really great.
Can you remove the .tsconfig file? With that, it should be all set.
I'll tag the current version as well to make it easier for people to use, and publish it to NPM.
package.json
Outdated
| "keywords": [ | ||
| "leaflet" | ||
| ], | ||
| "main": "./src/leaflet.functionaltilelayer.js", |
There was a problem hiding this comment.
The relative path that was there previously should have worked. Was it breaking for you in a particular case?
There was a problem hiding this comment.
I think my change was TypeScript-related. Reverted this.
|
Don't worry about the branch conflicts. I had added a version to the package.json and tagged it. I can handle merging your PR. |
…onaltilelayer * 'gh-pages' of https://github.com/ismyrnow/Leaflet.functionaltilelayer: Set version to 0.7.0
|
removed tsconfig. Thanks for merging! |
2 participants
No description provided.