Vislib Point Series updates #9044

ppisljar · 2016-11-11T09:52:31Z

Multiple changes to the Vislib point series charts. Before each of the chart (bar/area/line) would be an independent unit responsible for drawing all of its parts (axes, titles). This change splits things up so we can have greater control and better code reuse.

converting to ES6
moving axes out of each chart
joining both axes types (x and y) into a common axis type
allowing multiple axes
allowing top/bottom/left/right axis positioning
introducing new chart type 'point_series' which can combine bar/line/area series
making each of the series (bar/line/area) direction independent (vertical/horizontal charts)

This change should have no impact on the current way Kibana works/looks. All the visualizations should stay the same, no new options were added and the existing ones should keep working just as they did. But this will allow to implement many functionalities that are being asked for a lot like multiple Y axes, switching between line/area/bar chart easily, multiple chart types on one visualization, horizontal bar charts and heatmaps for example.

Fixes:
vislib performs width/height calculations redundantly #6555
Histogram bar hover only works once #5930
[vislib] Point series charts hould have bottom line #4067
"Container is too small" flashes briefly #3359

For posterity, this pr was submitted in several smaller prs: #8252, #8253, #8254, #8275, #8329, #8950, #8987

cjcenizal · 2016-11-11T22:12:16Z

src/ui/public/vislib/lib/layout/layout_types.js

    pie: Private(VislibLibLayoutTypesPieLayoutProvider),
-    tile_map: Private(VislibLibLayoutTypesMapLayoutProvider)
+    tile_map: Private(VislibLibLayoutTypesMapLayoutProvider),
+    point_series: Private(VislibLibLayoutTypesColumnLayoutProvider)


Is it safe to assume that point_series is now a base layout type for histogram, line, and area layouts?

yes, even more maybe. point series is now a base chart like pie. area, line and bars are just its series implementations.

cjcenizal · 2016-11-11T22:12:46Z

src/ui/public/vislib/lib/layout/types/column_layout.js

+                    type: 'div',
+                    class: 'x-axis-chart-title',
+                    splits: chartTitleSplit
+                  },*/


Can we delete this commented-out code?

cjcenizal · 2016-11-11T22:12:55Z

src/ui/public/vislib/lib/layout/types/column_layout.js

+                    type: 'div',
+                    class: 'y-axis-chart-title',
+                    splits: chartTitleSplit
+                  },*/


Can we delete this commented-out code?

cjcenizal · 2016-11-11T22:13:54Z

src/ui/public/vislib/lib/types/tile_map.js

+      this.charts.forEach(function (chart) {
+        chart.resizeArea();
+      });
+    };*/


Can we delete this commented-out code?

cjcenizal · 2016-11-11T22:15:44Z

src/ui/public/vislib/visualizations/point_series.js

+    };
+
+    draw() {
+      // todo: do we need to handle width and height here ?


Might want to uppercase TODO here for consistency with other TODOs in the code.

cjcenizal · 2016-11-11T22:19:02Z

src/ui/public/vislib/visualizations/point_series/_point_seri.js

+
+export default function PointSeriProvider(Private) {
+
+  class PointSeri {


Is this name a typo? I don't think "seri" is a word I've ever seen before (and I couldn't find any definition for it online), so the name doesn't help me understand the role of this class. Would just using "series" make sense for the role of this class?

This suggestion applies to the seri_types.js file too. That should be series_types I think.

Yeah, series is a troublesome word. What's the plural of series? serieses? it's a bit of a shock at first to me as well, but I think we might be better off just moving away from series entirely.

cjcenizal · 2016-11-11T22:31:12Z

This is amazing. It's such a huge PR, it's hard for me to wrap my head around so my comments are pretty limited. But from looking at the change in folder structure and how the code has been restructured, I can tell it's a little bit easier to dive into and understand (and scale).

I think two things could really help make vislib more accessible for everyone:

Clarify relationships between files/classes. This can be greatly improved by changing the import statements to be relative instead of absolute. This will help people trace the dependency graph more easily. Doing this will also help file/folder reorganization to reflect these relationships more clearly.
High-level overview on how vislib works. I'm just thinking of a simple outline or breakdown of concepts and parts involved in a VIS_LIB.md file in the vislib directory. It's really clear that you have a solid understand of how things work, and it would be great to share that knowledge.

Like I said, though... this is stellar. Thanks for doing this man. LGTM.

ppisljar · 2016-11-14T13:01:10Z

thanks @cjcenizal for looking into this. I updated based on your comments. the high level overview will need to get extended but it is a starting point i guess. let me know if you have specific questions you would like me to answer in there.

@spalger was reviewing my work along the way, thanks a lot for this again, so i think now it would be very useful to test this with diff datasets, there are probably things we didn't think off and its very important that this change in deed does work exactly as it did before for the end user.

thomasneirynck · 2016-11-15T00:31:01Z

this is obviously a little big to give detailed line-by-line feedback, so I'll try and summarize here:

I found the structure really legible. I think the design is a seriously good improvement. The addition of external design doc was really helpful to get started.
Apart from the good typing, the code is a lot cleaner. @spalger @ppisljar you guys rock.
there's a few todo-tags here in there that I would just remove from the code. This would even include the todos from the old vislib that you preserved. If there is not an external ticket tracking this, they are just going to continue to be mystifying.
I like the care you took on making API explicit and for example removing external access to "private" variables, and keeping the code flat. That really helps navigating through the code. For example, IntelliJ (and I assume many others) is quite good at type inference as long as you keep the OO simple, and the results are very noticeable with your improvements.

I tested this with existing line, bar-charts, and area-chart visualizations I had saved, and they continued to work. Creating, editing and saving these visualizations was not a problem.

As for whatever version this will go in, I think this needs to go through manual testing. I looked at the existing visualize-section of our tests, and it is already pretty extensive. But specifically, I think we should add manual tests for upgrading between Kibana versions of pre-vislib /post-vislib refactor, which is missing now. Right now, we have similar manual tests for 4->5 upgrades, in the context of the Dashboard. It will be beneficial to have a few specific ones for line/column/area-charts as well solely in the context of Visualize. Perhaps @LeeDr can offer some advice here.

tbragin · 2016-11-20T03:15:49Z

@ppisljar From a functional review standpoint, since our goal is to ensure no regressions occur as a result of this PR, in no particular order, I'll use this ticket to mention any differences I see between this PR on top of master and 5.0.1.

I don't know if any of the changes I notice is due to this PR or master, but nevertheless, here they go:

I noticed that CSV export has some weird characters in the beginning. I didn't see that in 5.0.1.
I noticed one partial bucket to pretty much always be there on the right for any date histogram where data stops. That does not seem to be the case for the same data and visualization on 5.0.1.
When selecting "Scaling Y-axis to data bounds", I see some lines cut off at the top. I didn't see that in 5.0.1.
Mouse-overs in pie charts don't seem to work. The "donut" option also doesn't appear to do anything. They work in 5.0.1.
Legends often cut off. I didn't see that in 5.0.1.

ppisljar · 2016-11-21T12:24:28Z

@tbragin thanks a lot for this !

CSV export issue I can't reproduce. Does it happen always for you ? or is it with some specific case ? could you please provide exact steps ?
partial bucket issue: should be fixed
scaling Y axis to data bounds: i can also reproduce this with latest master. the thing is that if you have two points which at a high scale end and you smooth the line it might actually go over the data bounds. we should probably handle that in separate PR (as it seems was not introduced by this change)
pie chart issues: should be fixed
axis legend cuts off: should be fixed i increased the truncate length to 100px.

LeeDr · 2016-11-21T14:31:55Z

Is the CSV export weird characters issue related to #8637

tbragin · 2016-11-21T14:39:25Z

@ppisljar

CSV export. I just exported the CSV and opened it in Excel for Mac 2011. However, I don't see the special character in vi, and it's not a character I can delete. Did we change anything about the way we generate CSV files as part of this PR?
Scaling -- Roger, and +1 on tracking as a separate issue and fix in separate PR.
The rest -- I'll try to pull down again and verify later today.

ppisljar · 2016-11-21T15:18:17Z

@LeeDr yeah it could be. We didnt' change anything around CSV in this PR as far as I'm aware

tbragin · 2016-11-21T16:12:40Z

@LeeDr @ppisljar Weird part is that export from 5.0.1 looks fine in Excel Mac 2011. But if we didn't change anything about CSV export in this PR, let's not worry about it.

ppisljar · 2016-11-21T16:16:51Z

i squashed the commit and rebased on master. let me know if anyone objects. (commits get squashed on merge anyway) ... the original history can still be seen here

tbragin · 2016-11-21T16:55:50Z

@ppisljar Pulled down latest changes, confirm partial bucket issue, pie chart legend, and legend cut off as addressed.

thomasneirynck · 2016-12-05T19:29:15Z

src/ui/public/vislib/__tests__/lib/layout/layout.js

@@ -12,6 +12,8 @@ import $ from 'jquery';
 import VislibLibLayoutLayoutProvider from 'ui/vislib/lib/layout/layout';
 import FixturesVislibVisFixtureProvider from 'fixtures/vislib/_vis_fixture';
 import PersistedStatePersistedStateProvider from 'ui/persisted_state/persisted_state';
+import VislibVisConfig from 'ui/vislib/lib/vis_config';


I think absolute is fine in this case. most unit tests in Kibana seem to use absolute paths

thomasneirynck · 2016-12-05T19:36:56Z

src/ui/public/vislib/__tests__/lib/layout/layout.js

-        expect($(vis.el).find('.x-axis-title').length).to.be(1);
+        expect($(vis.el).find('.x-axis-wrapper').length).to.be(2);
+        expect($(vis.el).find('.x-axis-div-wrapper').length).to.be(2);
+        expect($(vis.el).find('.x-axis-title').length).to.be(2);


I think the magic numbers are OK here. This is really a test for internal implementation details anyways.

moving y-axis methods inside axis class updating handler to use Axis for yAxis as well introducting 'type' property to differ between X and Y axis joining x_axis and y_axis into a single class axis - splitting it into 3 subclasses (Axis, AxisLabels, AxisScale) - converting to ES6 classes + style fixes - adding more customization options updating handler to work with new Axis class - allowing handler to have multiple category/value axes (array) converting axis_title to ES6 classes and making it work with new axis updating column layout to support left/right top/bottom positioning of axis - updating css min-widths to 1px (removing them breaks the code) as we dont want to reserve the space for axes that dont exist. introducing AxisConfig class adding ordered and values back to axis to make other parts of vislib working renaming axis.scale to axis.axisScale removing comments adding scale type config removing unnecesarry configs (never used) adding point series chart type updating all charts introducing VisConfig class moving wiggle, silluete and stacking to axis fixing based on last spencers review add label to zero filled values allow custom values on catwegory axis adding clip path updating defaults to match current vislib implementation seri.show parameter vertical category axis positioning fixing more issues fixing broken pie charts increasing default truncate length fixing expandLastBucket option

…gative values correctly)

Backports PR #9044 **Commit 1:** renaming x-axis to axis moving y-axis methods inside axis class updating handler to use Axis for yAxis as well introducting 'type' property to differ between X and Y axis joining x_axis and y_axis into a single class axis - splitting it into 3 subclasses (Axis, AxisLabels, AxisScale) - converting to ES6 classes + style fixes - adding more customization options updating handler to work with new Axis class - allowing handler to have multiple category/value axes (array) converting axis_title to ES6 classes and making it work with new axis updating column layout to support left/right top/bottom positioning of axis - updating css min-widths to 1px (removing them breaks the code) as we dont want to reserve the space for axes that dont exist. introducing AxisConfig class adding ordered and values back to axis to make other parts of vislib working renaming axis.scale to axis.axisScale removing comments adding scale type config removing unnecesarry configs (never used) adding point series chart type updating all charts introducing VisConfig class moving wiggle, silluete and stacking to axis fixing based on last spencers review add label to zero filled values allow custom values on catwegory axis adding clip path updating defaults to match current vislib implementation seri.show parameter vertical category axis positioning fixing more issues fixing broken pie charts increasing default truncate length fixing expandLastBucket option * Original sha: ba74498 * Authored by ppisljar <[email protected]> on 2016-09-13T10:59:17Z **Commit 2:** fixing selenium tests by increasing barHeightTolerance * Original sha: f27f8a1 * Authored by ppisljar <[email protected]> on 2016-11-22T09:40:25Z **Commit 3:** fixing axis alignment (1px off) * Original sha: 3d6267e * Authored by ppisljar <[email protected]> on 2016-11-22T12:19:37Z **Commit 4:** fixing layout elements min-height to 0 * Original sha: 619a495 * Authored by ppisljar <[email protected]> on 2016-11-22T12:20:20Z **Commit 5:** point radius should be calculated per chart * Original sha: be16b14 * Authored by ppisljar <[email protected]> on 2016-11-22T13:35:27Z **Commit 6:** adding clip path to circles * Original sha: 188131b * Authored by ppisljar <[email protected]> on 2016-11-22T13:43:55Z **Commit 7:** seting min height 0 on axis * Original sha: 231a58a * Authored by ppisljar <[email protected]> on 2016-11-22T14:45:12Z **Commit 8:** adding background class * Original sha: c41e672 * Authored by ppisljar <[email protected]> on 2016-11-22T15:27:02Z **Commit 9:** fixing selenium tests * Original sha: ed1b330 * Authored by ppisljar <[email protected]> on 2016-11-22T15:27:40Z **Commit 10:** update visualize legend to correctly check if it should show * Original sha: 9bd80be * Authored by ppisljar <[email protected]> on 2016-11-23T10:33:06Z **Commit 11:** fixing based on CJs comments * Original sha: 02a22d0 * Authored by ppisljar <[email protected]> on 2016-12-01T06:44:37Z **Commit 12:** improving stacking of negative values * Original sha: 9d79d79 * Authored by ppisljar <[email protected]> on 2016-12-01T06:55:33Z **Commit 13:** updating class name to better match element * Original sha: 628408d * Authored by ppisljar <[email protected]> on 2016-12-01T06:56:11Z **Commit 14:** fixing charts with mixed (negative/positive) values * Original sha: 682ab0b * Authored by ppisljar <[email protected]> on 2016-12-01T09:44:49Z **Commit 15:** fixing test (stacking happens for grouped charts as well to handle negative values correctly) * Original sha: 69a53ea * Authored by ppisljar <[email protected]> on 2016-12-01T09:52:29Z **Commit 16:** fixing based on CJs last comments * Original sha: 9e8d71f * Authored by ppisljar <[email protected]> on 2016-12-01T16:05:31Z **Commit 17:** fixing unstable selenium test * Original sha: f36b6fc * Authored by ppisljar <[email protected]> on 2016-12-06T11:20:30Z

ppisljar added Feature:Visualizations Generic visualization features (in case no more specific feature label is available) review Feature:Vislib Vislib chart implementation labels Nov 11, 2016

ppisljar force-pushed the vislib-axis-refactor branch from afb5976 to 3e08766 Compare November 11, 2016 10:09

ppisljar assigned thomasneirynck Nov 11, 2016

ppisljar changed the title ~~Vislib axis refactor~~ Vislib Point Series updates Nov 11, 2016

ppisljar force-pushed the vislib-axis-refactor branch from 3e08766 to 4b84d22 Compare November 11, 2016 10:35

cjcenizal reviewed Nov 11, 2016

View reviewed changes

ppisljar mentioned this pull request Nov 15, 2016

Secondary Y-Axis #7544

Closed

thomasneirynck mentioned this pull request Nov 15, 2016

Heatmap chart visualization #6974

Closed

ppisljar force-pushed the vislib-axis-refactor branch from f0a0595 to 23bf6d3 Compare November 21, 2016 16:12

ppisljar force-pushed the vislib-axis-refactor branch 2 times, most recently from d000edd to 6ae541d Compare November 22, 2016 19:25

ppisljar added the v5.2.0 label Nov 23, 2016

thomasneirynck approved these changes Dec 5, 2016

View reviewed changes

ppisljar added 16 commits December 5, 2016 22:20

fixing selenium tests by increasing barHeightTolerance

f27f8a1

fixing axis alignment (1px off)

3d6267e

fixing layout elements min-height to 0

619a495

point radius should be calculated per chart

be16b14

adding clip path to circles

188131b

seting min height 0 on axis

231a58a

adding background class

c41e672

fixing selenium tests

ed1b330

update visualize legend to correctly check if it should show

9bd80be

fixing based on CJs comments

02a22d0

improving stacking of negative values

9d79d79

updating class name to better match element

628408d

fixing charts with mixed (negative/positive) values

682ab0b

fixing test (stacking happens for grouped charts as well to handle ne…

69a53ea

…gative values correctly)

fixing based on CJs last comments

9e8d71f

ppisljar force-pushed the vislib-axis-refactor branch from b0ee1d0 to 9e8d71f Compare December 5, 2016 21:20

fixing unstable selenium test

f36b6fc

ppisljar merged commit b197945 into elastic:master Dec 6, 2016

elastic-jasper mentioned this pull request Dec 6, 2016

[5.x] Vislib Point Series updates #9388

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Vislib Point Series updates #9044

Vislib Point Series updates #9044

ppisljar commented Nov 11, 2016 •

edited

Loading

cjcenizal Nov 11, 2016

ppisljar Nov 14, 2016

cjcenizal Nov 11, 2016

cjcenizal Nov 11, 2016

cjcenizal Nov 11, 2016

cjcenizal Nov 11, 2016

cjcenizal Nov 11, 2016

spalger Nov 15, 2016

cjcenizal commented Nov 11, 2016 •

edited

Loading

ppisljar commented Nov 14, 2016 •

edited

Loading

thomasneirynck commented Nov 15, 2016

tbragin commented Nov 20, 2016 •

edited

Loading

ppisljar commented Nov 21, 2016 •

edited

Loading

LeeDr commented Nov 21, 2016

tbragin commented Nov 21, 2016

ppisljar commented Nov 21, 2016

tbragin commented Nov 21, 2016 •

edited

Loading

ppisljar commented Nov 21, 2016

tbragin commented Nov 21, 2016

thomasneirynck Dec 5, 2016

thomasneirynck Dec 5, 2016


		export default function PointSeriProvider(Private) {

		class PointSeri {

Vislib Point Series updates #9044

Vislib Point Series updates #9044

Conversation

ppisljar commented Nov 11, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjcenizal commented Nov 11, 2016 • edited Loading

ppisljar commented Nov 14, 2016 • edited Loading

thomasneirynck commented Nov 15, 2016

tbragin commented Nov 20, 2016 • edited Loading

ppisljar commented Nov 21, 2016 • edited Loading

LeeDr commented Nov 21, 2016

tbragin commented Nov 21, 2016

ppisljar commented Nov 21, 2016

tbragin commented Nov 21, 2016 • edited Loading

ppisljar commented Nov 21, 2016

tbragin commented Nov 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ppisljar commented Nov 11, 2016 •

edited

Loading

cjcenizal commented Nov 11, 2016 •

edited

Loading

ppisljar commented Nov 14, 2016 •

edited

Loading

tbragin commented Nov 20, 2016 •

edited

Loading

ppisljar commented Nov 21, 2016 •

edited

Loading

tbragin commented Nov 21, 2016 •

edited

Loading