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.

Sign up for GitHub

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

Jump to bottom

fix map zoom settings #17367

Merged
merged 3 commits into from
Mar 27, 2018
Merged

fix map zoom settings #17367

merged 3 commits into from
Mar 27, 2018

Conversation

thomasneirynck
Copy link
Contributor

@thomasneirynck thomasneirynck commented Mar 23, 2018 •
edited
Loading

Does a couple of things:

@thomasneirynck thomasneirynck added Feature:Visualizations Generic visualization features (in case no more specific feature label is available) release_note:fix v7.0.0 v6.3.0 v6.2.4 labels Mar 23, 2018
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

thomasneirynck
thomasneirynck commented Mar 23, 2018
View reviewed changes
src/ui/public/vis/map/service_settings.js
if (tilemapsConfig.deprecated.isOverridden) {//use tilemap.* settings from yml
const tmsService = _.cloneDeep(tmsOptionsFromConfig);
tmsService.url = tilemapsConfig.deprecated.config.url;
tmsService.id = 'Tilemap layer in yml';
tmsService.id = 'TMS in config/kibana.yml';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

made this a bit more descriptive

thomasneirynck
thomasneirynck commented Mar 23, 2018
View reviewed changes
src/ui/public/vis/map/service_settings.js
return allServices;

const servicesFromManifest = await this._loadTMSServices();
return allServices.concat(servicesFromManifest);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

merges both services (config/yml config as well as EMS-manifest config)

thomasneirynck
thomasneirynck commented Mar 23, 2018
View reviewed changes
src/core_plugins/tile_map/public/base_maps_visualization.js
@@ -167,12 +170,10 @@ export function BaseMapsVisualizationProvider(serviceSettings) {
}

async _setTmsLayer(tmsLayer) {
if (tmsLayer.maxZoom < this._kibanaMap.getMaxZoomLevel()) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

this was the bug. This check only worked in the wrong direction. ie. being in a current config with more zoom levels, and having to snap back because the new config has fewer zoom levels. But it should work the other way around as well. If you have more zoom levels, it should enable these too.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

nreese
nreese approved these changes Mar 27, 2018
View reviewed changes
Copy link
Contributor

@nreese nreese left a comment

Choose a reason for hiding this comment

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

lgtm

@thomasneirynck thomasneirynck merged commit ca70fb7 into elastic:master Mar 27, 2018
thomasneirynck added a commit to thomasneirynck/kibana that referenced this pull request Mar 27, 2018
@thomasneirynck
fix map zoom settings (elastic#17367)
d4f307a
@thomasneirynck thomasneirynck mentioned this pull request Mar 27, 2018
Merged
thomasneirynck added a commit to thomasneirynck/kibana that referenced this pull request Mar 27, 2018
@thomasneirynck
fix map zoom settings (elastic#17367)
e461c8f
@thomasneirynck thomasneirynck mentioned this pull request Mar 27, 2018
Merged
thomasneirynck added a commit that referenced this pull request Mar 27, 2018
@thomasneirynck
fix map zoom settings (#17367) (#17417)
1ab16f3
thomasneirynck added a commit that referenced this pull request Mar 27, 2018
@thomasneirynck
fix map zoom settings (#17367) (#17418)
9cc5c0d
@thomasneirynck thomasneirynck mentioned this pull request Apr 4, 2018
Closed
@kingigi kingigi mentioned this pull request Apr 19, 2018
Closed
@kingigi
Copy link

kingigi commented Apr 19, 2018

I am sorry to comment on a closed ticket, but are you sure that the problem with the higher zoom-levels was solved?
This ticket is tagged with v6.2.4, so I think the problem should be fixed in v6.2.4?
I received the RPM update to Kibana 6.2.4-1, but the zoom-level does not seem to be working on higher zoom-levels.

@thomasneirynck
Copy link
Contributor Author

@kingigi thank you, can confirm. Looking into it

@thomasneirynck
Copy link
Contributor Author

@kingigi sorry, made a mistake, was looking at wrong release.

Please ensure you have the layer selected: e.g.:

image

And make sure the kibana.yml is configured.
e.g.:

tilemap.url: 	https://a.tile.openstreetmap.org/{z}/{x}/{y}.png
tilemap.options.minZoom: 0
tilemap.options.maxZoom: 18

@kingigi
Copy link

kingigi commented Apr 21, 2018

@thomasneirynck : Check.
My visualizations are based on "TMS in config/kibana.yml".
And the mentioned timemap config-statements are my kibana.yml.
Nothing changed there.

@thomasneirynck
Copy link
Contributor Author

@kingigi you will have to resave your visualization. The max-zoom setting is saved with the saved-object in the .kibana index. Can you recreate the visualization and let me know if it persists? thx

@kingigi
Copy link

kingigi commented Apr 25, 2018

Hi @thomasneirynck,
Fixed!
I opened an re-saved the visualization. I didn't know I had to resave them when the zoom-level is changed in the configuration.
Thnx!
Egon

This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nreese nreese nreese approved these changes

@timroes timroes timroes approved these changes

Assignees
No one assigned
Labels
Feature:Coordinate Map Feature:Visualizations Generic visualization features (in case no more specific feature label is available) release_note:fix v6.2.4 v6.3.0 v7.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@thomasneirynck @elasticmachine @kingigi @nreese @timroes