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

properly render disabled range control #20811

Merged
merged 2 commits into from
Jul 17, 2018
Merged

properly render disabled range control #20811

merged 2 commits into from
Jul 17, 2018

Conversation

nreese
Copy link
Contributor

@nreese nreese commented Jul 14, 2018

partial fix for #20807

Update RangeControl to properly handle disabled state. Add jest tests to ensure proper rendering.

@nreese nreese mentioned this pull request Jul 14, 2018
Closed
@nreese nreese requested review from cjcenizal and liza-mae July 14, 2018 21:35
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@liza-mae
Copy link
Contributor

LGTM, test is now passing. Thanks for the quick turn around @nreese !

@liza-mae liza-mae mentioned this pull request Jul 15, 2018
Merged
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@stacey-gammon
Copy link
Contributor

What do you think about adding some new selenium tests to _embeddable_rendering that moves the time range to where there is no data and does a few checks like await dashboardExpect.pieSliceCount(0);. I think that would have caught this issue, and would also cover any other widgets that cause a fatal crash when there is no data.

There was another bug that would have been caught with this test too, so I think it would be helpful.

cjcenizal
cjcenizal approved these changes Jul 16, 2018
View reviewed changes
Copy link
Contributor

@cjcenizal cjcenizal left a comment

Choose a reason for hiding this comment

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

Code LGTM!

@nreese
Copy link
Contributor Author

nreese commented Jul 17, 2018

@stacey-gammon I am going to merge this without the additional functional tests. The jest tests provide good coverage that the components can be rendered when disabled and this fixes a big problem with the sample data set when the data loads with holes.

@nreese nreese merged commit ffa6109 into elastic:master Jul 17, 2018
@nreese nreese mentioned this pull request Jul 17, 2018
Merged
nreese added a commit to nreese/kibana that referenced this pull request Jul 17, 2018
@nreese
properly render disabled range control (elastic#20811)
b0337ce 
* properly render disabled range control

* remove disabled from existing elements
nreese added a commit that referenced this pull request Jul 17, 2018
@nreese
properly render disabled range control (#20811) (#20869)
8aa0984 
* properly render disabled range control

* remove disabled from existing elements
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@liza-mae liza-mae liza-mae approved these changes

@cjcenizal cjcenizal cjcenizal approved these changes

Assignees
No one assigned
Labels
Feature:Input Control Input controls visualization release_note:fix v6.4.0 v7.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@nreese @elasticmachine @liza-mae @stacey-gammon @cjcenizal