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

Add dynamic raster tiles support #12352

Merged
merged 2 commits into from
Feb 2, 2023
Merged

Add dynamic raster tiles support #12352

merged 2 commits into from
Feb 2, 2023

Conversation

stepankuzmin
Copy link
Contributor

@stepankuzmin stepankuzmin commented Nov 2, 2022

This PR adds methods for changing a raster tile source dynamically (e.g. setTiles, setUrl). Same as in vector tiles #8048.

Closes #11982

Launch Checklist

  • briefly describe the changes in this PR
  • write tests for all new functionality
  • document any changes to public APIs
  • apply changelog label ('bug', 'feature', 'docs', etc) or use the label 'skip changelog'
  • add an entry inside this element for inclusion in the mapbox-gl-js changelog: <changelog>add methods for changing a raster tile source dynamically (e.g. setTiles, setUrl).</changelog>

Copy link
Contributor

@rreusser rreusser left a comment

Choose a reason for hiding this comment

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

This looks great! I usually delete layers, then delete sources, switch the url, then re-add everything. This seems like a convenient method, and nothing jumps out as being out of place. The test looks good. Can you confirm you've given it a basic test to ensure it looks visually correct?

this.load(() => this.map.style._clearSource(this.id));
}

setSourceProperty(callback: Function) {
Copy link
Contributor

@rreusser rreusser Jan 31, 2023

Choose a reason for hiding this comment

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

Passing comment, no action needed: Interesting. This is a bit of a strange method. The only usage I see is:

source.setSourceProperty(() => {
source.attribution = 'OpenStreetMap';
});

So I think it's equivalent to:

source.attribution = 'OpenStreetMap';
source.reload()

Maybe it's just a placeholder in case the reload logic gets more complicated. Just jumped out as a bit odd since by the arguments, the method doesn't itself set anything.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @rreusser 👋

Thanks for the review! Previously I used setSourceProperty in the VectorTileSource's setTiles and setUrl methods, so it's more like a leftover. I've removed it 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good! At the very least, maybe wrapSourceMutation(() => {...}) would convey the intent more clearly, but removing works as well. 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature request] RasterTileSource.setTiles
2 participants