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

Increase options for tile url templates. #715

Merged
merged 2 commits into from
Jun 23, 2017
Merged

Conversation

manthey
Copy link
Contributor

@manthey manthey commented Jun 22, 2017

Before, we would translate {x} -> tile x, {y} -> tile y, {z} ->
tile layer, and {s} -> one of the subdomains defined in the layer.
This makes these case insensitive, supports an options $ prefix, and
allows several additional subdomain specifications. Specifically,
{s:abc}, {S:abc}, {a-c}, and {a,b,c} are all equivalent. If the
subdomain is in the template string, the layer's subdomain list is
ignored.

When we refactor the documentation for the tileLayer, this information
should be included in the subdomain and template string docs.

Before, we would translate `{x}` -> tile x, `{y}` -> tile y, `{z}` ->
tile layer, and `{s}` -> one of the subdomains defined in the layer.
This makes these case insensitive, supports an options `$` prefix, and
allows several additional subdomain specifications.  Specifically,
`{s:abc}`, `{S:abc}`, `{a-c}`, and `{a,b,c}` are all equivalent.  If the
subdomain is in the template string, the layer's subdomain list is
ignored.

When we refactor the documentation for the tileLayer, this information
should be included in the subdomain and template string docs.
@codecov-io
Copy link

codecov-io commented Jun 22, 2017

Codecov Report

Merging #715 into master will increase coverage by 0.02%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #715      +/-   ##
==========================================
+ Coverage   95.26%   95.29%   +0.02%     
==========================================
  Files          83       83              
  Lines        9000     9014      +14     
==========================================
+ Hits         8574     8590      +16     
+ Misses        426      424       -2
Impacted Files Coverage Δ
src/tileLayer.js 98% <100%> (+0.06%) ⬆️
src/mapInteractor.js 96.12% <0%> (+0.28%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ead485f...212b1f8. Read the comment docs.

Copy link
Member

@aashish24 aashish24 left a comment

Choose a reason for hiding this comment

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

LRTM but I did not look into checking regex it would be good if @jbeezley can also have a look at it.

This parses the tile url template once instead of on every tile, then
just does a string substitution per tile.  It also handles character
rangers specified in reverse (e.g., `{c-a}` is the same as `{a-c}`).
@manthey
Copy link
Contributor Author

manthey commented Jun 23, 2017

I refactored the code so that the regex and subdomains are only parsed once when the url is set instead of each time a tile is requested.

Copy link
Member

@aashish24 aashish24 left a comment

Choose a reason for hiding this comment

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

thanks, I felt that I was missing something in my last review. Looking just at the code change, it looks good to me.

@manthey manthey merged commit a7ecabc into master Jun 23, 2017
@manthey manthey deleted the subdomain-template-options branch June 23, 2017 18:10
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.

3 participants