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

Change TileMapServiceImageryProvider to use UrlTemplateImageryProvider #3460

Merged
merged 10 commits into from
Jan 27, 2016

Conversation

hpinkos
Copy link
Contributor

@hpinkos hpinkos commented Jan 25, 2016

Finishing up #3150
Part of #2814

  • Added more spec tests
  • Added back UrlTemplateImageryProvider.reinitialize

# Conflicts:
#	CHANGES.md
@@ -201,16 +193,7 @@ define([
*/
this.enablePickFeatures = defaultValue(options.enablePickFeatures, true);
Copy link
Contributor

Choose a reason for hiding this comment

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

This should just be true here, since options may be a promise and not have enablePickFeatures property. Then in reinitialize we should do this.enablePickFeatures = defaultValue(options.enablePickFeatures, this.enablePickFeatures); so that it keeps whatever the setting was if it's omitted from the reinitialize options.

hpinkos added 2 commits January 25, 2016 15:16
@hpinkos
Copy link
Contributor Author

hpinkos commented Jan 25, 2016

@mramato ready

this._pickFeaturesUrlParts = urlTemplateToParts(this._pickFeaturesUrl, pickFeaturesTags);

this._readyPromise = when.resolve(true);
this._readyPromise = this.reinitialize(options);
Copy link
Contributor

Choose a reason for hiding this comment

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

this.reinitialize should assign _readyPromise itself, rather than assigning it here. Otherwise, when someone calls reinitialize later, _readyPromise will not be set.

@mramato
Copy link
Contributor

mramato commented Jan 26, 2016

Just that one last comment. @kring I know you reviewed this in the original PR, but do you want to take a final look? If not, no big deal.

@TiffanyLu, thanks again for doing the brunt of the work here and thanks @hpinkos for finishing it up.

@kring
Copy link
Member

kring commented Jan 26, 2016

Sure, I should be able to take a look tomorrow.

@mramato
Copy link
Contributor

mramato commented Jan 26, 2016

Awesome, thanks.

@hpinkos if the other changes we discussed offline regarding joinURLs are ready soon, just PR them into this branch.

@hpinkos
Copy link
Contributor Author

hpinkos commented Jan 26, 2016

@mramato @kring ready!

@mramato
Copy link
Contributor

mramato commented Jan 26, 2016

Add a test for TMS to verify query parameters are preserved.

@hpinkos
Copy link
Contributor Author

hpinkos commented Jan 26, 2016

@mramato done

@kring
Copy link
Member

kring commented Jan 27, 2016

This all looks good to me!

@mramato
Copy link
Contributor

mramato commented Jan 27, 2016

Thanks @kring. Thanks again @hpinkos and @TiffanyLu

mramato added a commit that referenced this pull request Jan 27, 2016
Change TileMapServiceImageryProvider to use UrlTemplateImageryProvider
@mramato mramato merged commit cef460d into master Jan 27, 2016
@mramato mramato deleted the TMStoURL branch January 27, 2016 03:41
}

function requestMetadata() {
var resourceUrl = url + 'tilemapresource.xml';
Copy link
Contributor

Choose a reason for hiding this comment

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

@hpinkos We missed a spot during review (and apparently it's not covered by tests). This needs to use joinUrls too. Can you open a new PR with the fix and a test please. 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.

3 participants