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 zoom settings #11707

Merged
merged 5 commits into from
May 11, 2017
Merged

fix zoom settings #11707

merged 5 commits into from
May 11, 2017

Conversation

thomasneirynck
Copy link
Contributor

@thomasneirynck thomasneirynck commented May 10, 2017 •
edited
Loading

Closes #11697: when using WMS, there are no restrictions on zoom level (this is a regression in 5.4).

@thomasneirynck thomasneirynck added Feature:Visualizations Generic visualization features (in case no more specific feature label is available) release_note:fix v5.4.1 labels May 10, 2017
@thomasneirynck
Copy link
Contributor Author

jenkins, test this

@bhavyarm bhavyarm self-requested a review May 10, 2017 20:42
@thomasneirynck
Copy link
Contributor Author

jenkins, test this

bhavyarm
bhavyarm reviewed May 10, 2017
View reviewed changes
Copy link
Contributor

@bhavyarm bhavyarm left a comment

Choose a reason for hiding this comment

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

With x-pack license trial - max zoom is working only up to 10.

@thomasneirynck thomasneirynck requested a review from ppisljar May 11, 2017 02:03
@thomasneirynck
Copy link
Contributor Author

thomasneirynck commented May 11, 2017 •
edited
Loading

@bhavyarm you are right, and I had a misremembered the original reason for deprecating the maxZoom setting. We had deprecated the maxZoom setting in 5.2, so users could not manually configure the elastic service.

I removed that part of the PR (bf2de09), since it wasn't introduced in 5.4 either.

@ppisljar
Copy link
Member

ppisljar commented May 11, 2017 •
edited
Loading

when using WMS its now possible to zoom in more than there are zoom levels available ... for example with default basemap.nationalmap.gov there are 16 zoom levels available but you can zoom in past that (and get blank screen).

would it be possible to detect the available zoom levels ?
or at least detect when we are not getting anything back and show an error at that point.

@thomasneirynck
Copy link
Contributor Author

test fails on upload to S3.

jenkins, test this

@thomasneirynck
Copy link
Contributor Author

@ppisljar It depends on the service if this is possible. Some WMS service advertise the max scale for which it can produce data, but this is an optional parameter and many WMS services do not implement this correctly. It would also require the WMS to imlement CORS, which is not a given. It would be a nice-to-have I agree, but that functionality would be out of scope for this PR.

@ppisljar
Copy link
Member

could we show an error when we are not getting any data, instead of just white screen ? (and block further zooming in once we reach a level which throws error ?)

ppisljar
ppisljar approved these changes May 11, 2017
View reviewed changes
Copy link
Member

@ppisljar ppisljar 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 c946b43 into elastic:master May 11, 2017
thomasneirynck added a commit to thomasneirynck/kibana that referenced this pull request May 11, 2017
@thomasneirynck
When using WMS, the zoom settings of the manifest should not be used (e…
adee793 
…lastic#11707)

When a user configures a WMS, we should not use the zoom-settings from the manifest. These depend on the user's license level, and are not relevant when using a 3rd party WMS service.
thomasneirynck added a commit to thomasneirynck/kibana that referenced this pull request May 11, 2017
@thomasneirynck
When using WMS, the zoom settings of the manifest should not be used (e…
3f17fec 
…lastic#11707)

When a user configures a WMS, we should not use the zoom-settings from the manifest. These depend on the user's license level, and are not relevant when using a 3rd party WMS service.
This was referenced May 11, 2017
Merged
Merged
@thomasneirynck
Copy link
Contributor Author

backports:
5.x: #11729
5.4: #11730

thomasneirynck added a commit that referenced this pull request May 11, 2017
@thomasneirynck
When using WMS, the zoom settings of the manifest should not be used (#…
bd77356 
…11707) (#11729)

When a user configures a WMS, we should not use the zoom-settings from the manifest. These depend on the user's license level, and are not relevant when using a 3rd party WMS service.
thomasneirynck added a commit that referenced this pull request May 11, 2017
@thomasneirynck
When using WMS, the zoom settings of the manifest should not be used (#…
b665880 
…11707) (#11730)

When a user configures a WMS, we should not use the zoom-settings from the manifest. These depend on the user's license level, and are not relevant when using a 3rd party WMS service.
snide pushed a commit to snide/kibana that referenced this pull request May 30, 2017
@thomasneirynck @snide
When using WMS, the zoom settings of the manifest should not be used (e…
74c6e81 
…lastic#11707)

When a user configures a WMS, we should not use the zoom-settings from the manifest. These depend on the user's license level, and are not relevant when using a 3rd party WMS service.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bhavyarm bhavyarm bhavyarm left review comments

@ppisljar ppisljar ppisljar approved these changes

Assignees
No one assigned
Labels
Feature:Visualizations Generic visualization features (in case no more specific feature label is available) release_note:fix v5.4.1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@thomasneirynck @ppisljar @bhavyarm