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

Change the scale factor when upgrading panels with margins #20684

Merged
merged 2 commits into from
Jul 12, 2018
Merged

Change the scale factor when upgrading panels with margins #20684

merged 2 commits into from
Jul 12, 2018

Conversation

trevan
Copy link
Contributor

@trevan trevan commented Jul 11, 2018

Fixes #20635

This reduces the scale factor to 4 when migrating panels that are in a dashboard with margins.

@elasticmachine
Copy link
Contributor

Since this is a community submitted pull request, a Jenkins build has not been kicked off automatically. Can an Elastic organization member please verify the contents of this patch and then kick off a build manually?

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
code review, tested in chrome by creating a dashboard with margins. Simulated 6.2.0 dashboards by modifying saved object panelsJSON for each panel by setting version to 6.2.0 and dividing x and w attributes by 4 and dividing y and h attributes by 5. I then loaded the dashboard objects to view their appearance.

@nreese
Copy link
Contributor

nreese commented Jul 11, 2018

A couple of screen shots from reviewing.

Loading a 6.2.0 dashboard with this PR (scaling factor of 4)
screen shot 2018-07-11 at 2 03 12 pm

Loading a 6.2.0 dashboard on master (scaling factor of 5)
screen shot 2018-07-11 at 2 03 49 pm

@stacey-gammon
Copy link
Contributor

Nice reviewing @nreese . I did something similar but created dashboard in 6.2, then exported and imported in 6.3, once without this PR, then once with.

6.2:
screen shot 2018-07-11 at 4 03 01 pm

6.3 with this PR (ignore the not found errors, didn't bother with data):
screen shot 2018-07-11 at 4 09 31 pm

6.3 on master
screen shot 2018-07-11 at 4 10 28 pm

Def a lot closer with this PR.

Code lgtm too.

@stacey-gammon
Copy link
Contributor

retest

@elasticmachine
Copy link
Contributor

💔 Build Failed

@nreese
Copy link
Contributor

nreese commented Jul 11, 2018

Looks like a legitimate test failure in dashboard_grid_container.test.js

loads old panel data in the right order
20:40:38 
20:40:38     expect(received).toBe(expected) // Object.is equality
20:40:38     
20:40:38     Expected value to be:
20:40:38       35
20:40:38     Received:
20:40:38       28
20:40:38 
20:40:38       102 |   expect(foo8Panel.row).toBe(undefined);
20:40:38       103 |   expect(foo8Panel.gridData.y).toBe(35);
20:40:38     > 104 |   expect(foo8Panel.gridData.x).toBe(0);
20:40:38       105 | 
20:40:38       106 |   grid.unmount();
20:40:38       107 | });

@trevan
Copy link
Contributor Author

trevan commented Jul 12, 2018

That test was using useMargins = true because it is the default value in the store. I added a second test to check when useMargins = false and fixed the expected value.

@stacey-gammon
Copy link
Contributor

retest

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@nreese nreese merged commit db7c08d into elastic:master Jul 12, 2018
nreese pushed a commit to nreese/kibana that referenced this pull request Jul 12, 2018
…0684)

* Change the scale factor when upgrading panels with margins

* Add test for dashboard_grid_container with and without margins
@nreese
Copy link
Contributor

nreese commented Jul 12, 2018

6.x #20726
6.3 #20727

nreese added a commit that referenced this pull request Jul 12, 2018
…20726)

* Change the scale factor when upgrading panels with margins

* Add test for dashboard_grid_container with and without margins
nreese added a commit that referenced this pull request Jul 12, 2018
) (#20727)

* merge conflicts

* remove grid_container test with margins
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.

4 participants