[input controls] Horizontal layout #14918

nreese · 2017-11-13T17:45:11Z

Dynamically configure the input control panel layout based on the panel dimensions. When the panel is short and long - lay the controls out in a horizontal fashion. When the panel is tall and skinny - lay the controls out in a vertical fashion

stacey-gammon · 2017-11-13T18:38:27Z

I suspect a flaky test:

27:08          │ debg  clickByCssSelector([href="#/visualize"])
18:27:08          │ debg  findByCssSelector [href="#/visualize"]
18:27:08          │ debg  clickVisualizationByLinkText(Visualization TileMap)
18:27:19          │ debg  --- tryForTime failure: [POST http://localhost:9515/session/f64b0bb49a0a40eeabeb455dc1452c97/element / {"using":"partial link text","value":"Visualization TileMap"}] no such element: Unable to locate element: {"method":"partial link text","selector":"Visualization TileMap"}
18:27:19          │         (Session info: chrome=62.0.3202.75)
18:27:19          │         (Driver info: chromedriver=2.33.506092 (733a02544d189eeb751fe0d7ddca79a0ee28cce4),platform=Linux 4.4.0-45-generic x86_64)
18:27:29          │ debg  --- tryForTime failed again with the same message  ...
18:27:40          │ debg  --- tryForTime failed again with the same message  ...
18:27:50          │ debg  --- tryForTime failed again with the same message  ...
18:27:51          │ debg  Taking screenshot "/var/lib/jenkins/workspace/elastic+kibana+pull-request+multijob-selenium/test/functional/screenshots/failure/visualize app tile map visualize app tile map chart should save with zoom level and load, take screenshot.png"
18:27:51        └- ✖ fail: "visualize app tile map visualize app tile map chart should save with zoom level and load, take screenshot"
18:27:51        │        tryForTime timeout: [POST http://localhost:9515/session/f64b0bb49a0a40eeabeb455dc1452c97/element / {"using":"partial link text","value":"Visualization TileMap"}] no such element: Unable to locate element: {"method":"partial link text","selector":"Visualization TileMap"}
18:27:51        │         (Session info: chrome=62.0.3202.75)
18:27:51        │         (Driver info: chromedriver=2.33.506092 (733a02544d189eeb751fe0d7ddca79a0ee28cce4),platform=Linux 4.4.0-45-generic x86_64)
18:27:51        │           at Server._post (/var/lib/jenkins/workspace/elastic+kibana+pull-request+multijob-

screenshot:

cc @elastic/kibana-visualizations, does this look like a flaky test to you at all? I'm pretty sure I've seen this one before.

jenkins, test this

thomasneirynck · 2017-11-13T18:40:00Z

@nreese yeah, this test seems to have been failing every once in a while, the last couple of weeks. I can take a look. fyi I created #14920.

stacey-gammon · 2017-11-13T18:54:30Z

src/core_plugins/input_control_vis/public/vis_controller.js

+    }
+    const panelWidth = panelNode.clientWidth;
+    const panelHeight = panelNode.clientHeight;
+    // todo - how do we make this less fragile to css changes?


maybe we need something exposed to the embeddable from dashboard - width/height that is only the inner values, and an onResize function.

I filed #14921 so I can keep track of these kinds of things. Plus then maybe we can remember to go back and clean this up, since I agree, it's going to be very fragile.

What about adding a selenium test to try to ensure we don't break it accidentally? I'm dubious whether the react test would catch any issues. I've had problems testing dimension values in the jest tests because jsdom returns all 0 for getClientBoundingRect (or whatever that function is called).

stacey-gammon

Some minor feedback, but lgtm.

stacey-gammon · 2017-11-14T13:57:08Z

test/functional/apps/dashboard/_dashboard_grid.js

+            .pressMouseButton()
+            .moveMouseTo(null, 300, 0)
+            .releaseMouseButton();
+          await PageObjects.common.sleep(500); //give time for vis to redraw


I try to avoid sleeps as much as possible - what about wrapping the below in a retry, so if it fails the first time, I'll try again, and by then enough time should have passed so it finished redrawing? (here and below function)

retry does not work in this situation since the elements already exist and will be returned the first attempt. A sleep is needed before the expect to allow the elements to be re-drawn with the new state. I updated the commits around the sleep to reflect this.

stacey-gammon · 2017-11-14T13:58:03Z

test/functional/apps/dashboard/_dashboard_grid.js

+        before(async () => {
+          await PageObjects.common.navigateToApp('dashboard');
+          await PageObjects.dashboard.clickNewDashboard();
+          await PageObjects.dashboard.addVisualizations(['Visualization InputControl']);


sweeeet, now you're going to have to show me how to add more visualization types to the archive. :)

stacey-gammon · 2017-11-14T14:17:35Z

src/core_plugins/input_control_vis/public/vis.less

@@ -44,5 +44,9 @@ visualization.input_control_vis {
  }
 }

+visualize.input_control_vis {
+  padding: 0 !important;


I think you should add a comment here explaining why this is important (I'm guessing bc of the width calculations?)

stacey-gammon · 2017-11-14T14:18:32Z

src/core_plugins/input_control_vis/public/vis_controller.js

+const panelMargin = 1;
+const embeddedPanelPadding = 8;
+const inputControlVisMargin = 5;
+const flexGridSmallMargin = 4;


tiny nit but I think as static const variables, these are supposed to be named like EMBEDDED_PANEL_PADDING. Also, is there a file you grabbed these from? Might be worthwhile to point to it so if anything looks off someone can double check the values match and know where to look. (though who knows how long that will stay in sync for... just a suggestion, take it or leave it)

kobelb

This is behaving really weirdly:

nreese · 2017-11-27T16:58:34Z

@stacey-gammon @kobelb I have refactor this PR to use the ui framework's KuiFlexGroup with the wrap option (thanks @cjcenizal ). This avoids the entire width calculation funny business. Please take another look.

kobelb

LGTM

kobelb · 2017-11-27T17:50:49Z

The CI failure looks unrelated, but I'd much prefer a green CI run before merging as it could be caused by this PR.

✖ fail: "dashboard app dashboard state Directly modifying url updates dashboard state for panel size parameters"

nreese · 2017-11-27T18:10:06Z

jenkins, test this

ppisljar · 2017-11-28T07:20:31Z

jenkins, test this

* input controls horizontal layout * fix controlWidth calculation * add functional test to ensure panel resizing changes layout * use all caps for consts, add more comments about where values came from * replace sleeps in functional tests with retry * use KuiFlexGroup with wrap option instead of manually calculating widths * remove no longer used min width constants

* [input controls] Horizontal layout (#14918) * input controls horizontal layout * fix controlWidth calculation * add functional test to ensure panel resizing changes layout * use all caps for consts, add more comments about where values came from * replace sleeps in functional tests with retry * use KuiFlexGroup with wrap option instead of manually calculating widths * remove no longer used min width constants * fix dashboard grid function test.

* input controls horizontal layout * fix controlWidth calculation * add functional test to ensure panel resizing changes layout * use all caps for consts, add more comments about where values came from * replace sleeps in functional tests with retry * use KuiFlexGroup with wrap option instead of manually calculating widths * remove no longer used min width constants

nreese added :Sharing release_note:enhancement v6.1.0 v7.0.0 labels Nov 13, 2017

nreese requested review from kobelb and stacey-gammon November 13, 2017 17:45

stacey-gammon reviewed Nov 13, 2017

View reviewed changes

stacey-gammon mentioned this pull request Nov 13, 2017

Embeddables: Expose inner width/height and onResize #14921

Closed

stacey-gammon approved these changes Nov 14, 2017

View reviewed changes

kobelb reviewed Nov 16, 2017

View reviewed changes

nreese force-pushed the horizontal_layout branch from 06c466b to 6458a5e Compare November 16, 2017 17:33

nreese added v6.2.0 and removed v6.1.0 labels Nov 21, 2017

nreese force-pushed the horizontal_layout branch from 6458a5e to 115cc59 Compare November 27, 2017 15:55

kobelb approved these changes Nov 27, 2017

View reviewed changes

nreese force-pushed the horizontal_layout branch from ec42357 to 6338a3d Compare November 28, 2017 15:01

nreese added 7 commits November 28, 2017 08:05

input controls horizontal layout

29fe265

fix controlWidth calculation

b1d5ca0

add functional test to ensure panel resizing changes layout

ba811a9

use all caps for consts, add more comments about where values came from

09f1c45

replace sleeps in functional tests with retry

eac8041

use KuiFlexGroup with wrap option instead of manually calculating widths

acdc425

remove no longer used min width constants

198ab11

nreese force-pushed the horizontal_layout branch from 6338a3d to 198ab11 Compare November 28, 2017 15:08

nreese merged commit c579677 into elastic:master Nov 29, 2017

This was referenced Nov 29, 2017

[Backport] [input controls] Horizontal layout (#14918) #15263

Closed

[6.x] [input controls] Horizontal layout (#14918) #15307

Merged

nreese deleted the horizontal_layout branch February 13, 2018 19:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[input controls] Horizontal layout #14918

[input controls] Horizontal layout #14918

nreese commented Nov 13, 2017

stacey-gammon commented Nov 13, 2017

thomasneirynck commented Nov 13, 2017 •

edited

Loading

stacey-gammon Nov 13, 2017

stacey-gammon Nov 13, 2017

stacey-gammon left a comment

stacey-gammon Nov 14, 2017

nreese Nov 14, 2017

stacey-gammon Nov 14, 2017

stacey-gammon Nov 14, 2017

stacey-gammon Nov 14, 2017

kobelb left a comment

nreese commented Nov 27, 2017

kobelb left a comment

kobelb commented Nov 27, 2017

nreese commented Nov 27, 2017

ppisljar commented Nov 28, 2017

[input controls] Horizontal layout #14918

[input controls] Horizontal layout #14918

Conversation

nreese commented Nov 13, 2017

stacey-gammon commented Nov 13, 2017

thomasneirynck commented Nov 13, 2017 • edited Loading

stacey-gammon Nov 13, 2017

Choose a reason for hiding this comment

stacey-gammon Nov 13, 2017

Choose a reason for hiding this comment

stacey-gammon left a comment

Choose a reason for hiding this comment

stacey-gammon Nov 14, 2017

Choose a reason for hiding this comment

nreese Nov 14, 2017

Choose a reason for hiding this comment

stacey-gammon Nov 14, 2017

Choose a reason for hiding this comment

stacey-gammon Nov 14, 2017

Choose a reason for hiding this comment

stacey-gammon Nov 14, 2017

Choose a reason for hiding this comment

kobelb left a comment

Choose a reason for hiding this comment

nreese commented Nov 27, 2017

kobelb left a comment

Choose a reason for hiding this comment

kobelb commented Nov 27, 2017

nreese commented Nov 27, 2017

ppisljar commented Nov 28, 2017

thomasneirynck commented Nov 13, 2017 •

edited

Loading