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

corrected spy fullscreen mode #8844

Merged
merged 2 commits into from
Oct 28, 2016
Merged

Conversation

scampi
Copy link
Contributor

@scampi scampi commented Oct 26, 2016

close #8843

2016-10-26-102453_1916x1041_scrot

@elasticmachine
Copy link
Contributor

Can one of the admins verify this patch?

Copy link
Contributor

@thomasneirynck thomasneirynck left a comment

Choose a reason for hiding this comment

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

I tested this in the Dashboard and the Visualize app, and the fix works well! Expanding the spy panel to full size, and collapsing it again now works as expected.

I don't think we need the guard-clauses to check if $spyEl exists. Each vis-container should have both the spy panel and the visualization. I removed those if-statements and the fix continues to work. Did you run into any issues without them?

@@ -76,13 +76,20 @@ uiModules

let applyClassNames = function () {
let $visEl = getVisContainer();
const $spyEl = getter('.visualize-spy-container')();
Copy link
Member

Choose a reason for hiding this comment

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

what do you think about defining getSpyElement in a way getVisContainer is defined just for consistency reasons ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

getSpyContainer would be more consistent though ;o)

@ppisljar
Copy link
Member

im wondering the same @thomasneirynck is wondering .... left a small comment

@ppisljar
Copy link
Member

jenkins, test this

@scampi
Copy link
Contributor Author

scampi commented Oct 28, 2016

@ppisljar @thomasneirynck The guard clauses were added only to account for the possibility of getter() returing undefined. I wrote that code a while back before doing this PR, so I am not 100% sure why I needed that check otherwise. I'll try without as well.

@scampi
Copy link
Contributor Author

scampi commented Oct 28, 2016

@ppisljar @thomasneirynck ready for review

@ppisljar
Copy link
Member

LGTM

@ppisljar
Copy link
Member

thanks a lot for fixing this @scampi

@thomasneirynck thomasneirynck merged commit 8f1d6e7 into elastic:master Oct 28, 2016
elastic-jasper added a commit that referenced this pull request Oct 28, 2016
---------

**Commit 1:**
corrected spy fullscreen mode

* Original sha: a092f15
* Authored by Stéphane Campinas <[email protected]> on 2016-10-26T09:22:58Z

**Commit 2:**
removed guard-clauses

* Original sha: 0403470
* Authored by Stéphane Campinas <[email protected]> on 2016-10-28T09:02:23Z
thomasneirynck pushed a commit that referenced this pull request Oct 28, 2016
Ensures that expanding the spy panel takes up the full width/height of the container. 

Closes #8843.

---------

**Commit 1:**
corrected spy fullscreen mode

* Original sha: a092f15
* Authored by Stéphane Campinas <[email protected]> on 2016-10-26T09:22:58Z

**Commit 2:**
removed guard-clauses

* Original sha: 0403470
* Authored by Stéphane Campinas <[email protected]> on 2016-10-28T09:02:23Z
@epixa epixa added v5.1.1 and removed v5.1.0 labels Dec 8, 2016
airow pushed a commit to airow/kibana that referenced this pull request Feb 16, 2017
Ensures that expanding the spy panel takes up the full width/height of the container. 

Closes elastic#8843.

---------

**Commit 1:**
corrected spy fullscreen mode

* Original sha: 7c208b94c47a5377753fb381030cd787c4bcc80c [formerly a092f15]
* Authored by Stéphane Campinas <[email protected]> on 2016-10-26T09:22:58Z

**Commit 2:**
removed guard-clauses

* Original sha: 1ed4ccf49d3bbc2734ba7633114221e9be49b8f3 [formerly 0403470]
* Authored by Stéphane Campinas <[email protected]> on 2016-10-28T09:02:23Z

Former-commit-id: 2fdf95a
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.

fullscreen spy is not really fullscreen
5 participants