Fix pushed down chart on multi line horizontal legends #31466

flash1293 · 2019-02-19T13:17:19Z

Summary

Fixes the issue with multi line horizontal legends which push down the chart into the x axis (see discussion in #30960)

This is fixed by rendering the visualization a second time after refreshing the legend after the first render:

if (this.vis.params.addLegend) {	
          this.$scope.refreshLegend++;	
          this.$scope.$digest();	
          this.vis.vislibVis.render(esResponse, this.vis.getUiState());
}

Checklist

Use ~~strikethroughs~~ to remove checklist items you don't feel are applicable to this PR.

This was checked for cross-browser compatibility, including a check against IE11
~~- [ ] Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support~~
~~- [ ] Documentation was added for features that require explanation or tutorials~~
~~- [ ] Unit or functional tests were updated or added to match the most common scenarios~~
~~- [ ] This was checked for keyboard-only and screenreader accessibility~~

For maintainers

~~- [ ] This was checked for breaking API changes and was labeled appropriately~~
~~- [ ] This includes a feature addition or change that requires a release note and was labeled appropriately~~

elasticmachine · 2019-02-19T14:02:21Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-02-19T15:26:28Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-02-19T16:19:21Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-02-19T17:10:39Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

ppisljar · 2019-02-21T13:31:09Z

src/legacy/ui/public/vis/vis_types/vislib_vis_type.js

+
+          // re-render after the legend is initialized to correctly take
+          // the legend height into account
+          this.vis.vislibVis.render(esResponse, this.vis.getUiState());


why was this working for the left/right legend ? we have the same scenario, where chart needs to take new width into account ?

I think the left/right legend has a fixed width and is taken correctly into account even if there are no entries in the legend in the first render pass

ok ... so before the logic was

render the legend (so chart knows the size)

render the chart

render the legend again with correct data (which chart prepared)

now:

render legend

render chart

render legend with correct data

render chart with correct size

.... this feels a bit too much, but i agree its a working easy fix

couldn't we just:

render chart

render legend with correct data

render chart

(this way we get rid of one legend render)
or even better, extract the logic in chart used to produce correct legend info, or separate the chart in two parts .. prepare data and render

then:

chart prepare data

render legend (with correct data)

chart render

chart prepare data

render legend (with correct data)

chart render

I tried to do this at first, but a lot of tests failed because of it. I will restore the state so we can take a look on it.

elasticmachine · 2019-02-21T15:38:59Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

flash1293 · 2019-02-22T08:57:34Z

I don't understand why this test is failing- it works for me if I run the test locally. Any ideas @ppisljar ? If the double pass rendering isn't too bad for performance (thinking about big dashboards), I will revert to re-rendering.

elasticmachine · 2019-02-22T09:44:53Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-02-22T13:54:07Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

flash1293 · 2019-02-22T13:59:34Z

Jenkins, test this.

elasticmachine · 2019-02-22T14:45:35Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2019-02-22T16:30:19Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

flash1293 · 2019-02-24T21:17:01Z

@timroes @ppisljar could you take another look? it now does the following:

Prepare data for legend
Render legend
Render chart
Re-render legend (because of heatmap and gauge)

Heatmap and gauge do some calculations during the rendering which is required to build the legend correctly. This means the multi line bug isn't fixed for those chart types (which is an edge case IMHO because you rarely have this many legend entries in those chart types plus the labels tend to be very short).

Refactoring the control flow here to also fix heatmap and gauge would require effort which IMHO isn't worth it at the moment because we will start using elastic-charts here soon.

elasticmachine · 2019-02-24T21:43:56Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

ppisljar · 2019-02-25T09:10:04Z

src/legacy/ui/public/vis/vis_types/vislib_vis_type.js

        this.vis.vislibVis.render(esResponse, this.vis.getUiState());

+        // refreshing the legend after the chart is rendered.
+        // this is necessary because some visualizations (heatmap and gauge)
+        // provide data necessary for the legend only after a render cycle.
        if (this.vis.params.addLegend) {


do this only for heatmap and gauge.
also, it seems that for heatmap and gauge we also need to rerender the chart again to fix top/bottom legend overflow

do this only for heatmap and gauge

Does it make sense to make this code specific to certain chart types? If a plugin vis uses the same "hack", its legend wouldn't show up at all.

also, it seems that for heatmap and gauge we also need to rerender the chart again to fix top/bottom legend overflow

Yes, see my comment above regarding that. It's really edge-casey to trigger this bug for heatmap and gauge and I wasn't sure whether it makes sense to hurt performance for all heatmaps and gauges for this. If you think that's reasonable I will add a re render just for these chart types.

if a third party plugin would want to do something similar, they would need to modify the vislib legend anyway as heatmap and gauge special legend handling is hardcoded in there.

can we try offload the decision if to rerender or not to the legend itself ?

i agree hitting the bug with heatmap or gauge is less likely, but on dashboard it might still happen quite often. And since the solution is anyway a hack, it would be nice if it actually fully fixes a problem.

if a third party plugin would want to do something similar, they would need to modify the vislib legend anyway as heatmap and gauge special legend handling is hardcoded in there.

Ah, you are right. I moved the list of "special" visualizations to a central constant to emphasise the connection.

elasticmachine · 2019-02-25T13:31:41Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

ppisljar

LGTM, tested on chrome linux

flash1293 added v7.0.0 v8.0.0 v6.7.0 v7.2.0 labels Feb 19, 2019

flash1293 requested review from timroes and ppisljar February 19, 2019 13:17

flash1293 force-pushed the 17488/fix-multiline-legend branch from 422d15d to 43e3dc6 Compare February 19, 2019 16:27

ppisljar reviewed Feb 21, 2019

View reviewed changes

flash1293 added 2 commits February 22, 2019 09:58

refresh chart after legend render

2b8deb7

other approach

f83c14c

flash1293 force-pushed the 17488/fix-multiline-legend branch from 1762080 to f83c14c Compare February 22, 2019 08:58

fix test

b1e24c8

add exception for gauge and heatmap

93ebcd5

add comment

10db798

ppisljar reviewed Feb 25, 2019

View reviewed changes

re render for defined list of visualizations

7f84f4e

flash1293 requested a review from ppisljar February 26, 2019 09:02

ppisljar approved these changes Feb 27, 2019

View reviewed changes

flash1293 merged commit 295de1e into elastic:master Feb 27, 2019

flash1293 deleted the 17488/fix-multiline-legend branch February 27, 2019 14:30

flash1293 added a commit to flash1293/kibana that referenced this pull request Feb 27, 2019

Fix pushed down chart on multi line horizontal legends (elastic#31466)

3437b1c

This was referenced Feb 27, 2019

[7.x] Fix pushed down chart on multi line horizontal legends (#31466) #32122

Merged

[7.0] Fix pushed down chart on multi line horizontal legends (#31466) #32123

Merged

flash1293 added a commit to flash1293/kibana that referenced this pull request Feb 27, 2019

Fix pushed down chart on multi line horizontal legends (elastic#31466)

96b88f9

flash1293 mentioned this pull request Feb 27, 2019

[6.7] Fix pushed down chart on multi line horizontal legends (#31466) #32128

Merged

flash1293 added a commit to flash1293/kibana that referenced this pull request Feb 27, 2019

Fix pushed down chart on multi line horizontal legends (elastic#31466)

0671cdf

flash1293 added a commit that referenced this pull request Mar 1, 2019

Fix pushed down chart on multi line horizontal legends (#31466) (#32128)

06e4388

flash1293 added a commit that referenced this pull request Mar 1, 2019

Fix pushed down chart on multi line horizontal legends (#31466) (#32122)

698953d

flash1293 added a commit that referenced this pull request Mar 1, 2019

Fix pushed down chart on multi line horizontal legends (#31466) (#32123)

815f0a5

timroes mentioned this pull request Mar 15, 2019

X-Axis misplacement in Line Visualization #33145

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix pushed down chart on multi line horizontal legends #31466

Fix pushed down chart on multi line horizontal legends #31466

flash1293 commented Feb 19, 2019 •

edited by timroes

Loading

elasticmachine commented Feb 19, 2019

elasticmachine commented Feb 19, 2019

elasticmachine commented Feb 19, 2019

elasticmachine commented Feb 19, 2019

ppisljar Feb 21, 2019

flash1293 Feb 21, 2019

ppisljar Feb 21, 2019

flash1293 Feb 21, 2019 •

edited

Loading

elasticmachine commented Feb 21, 2019

flash1293 commented Feb 22, 2019

elasticmachine commented Feb 22, 2019

elasticmachine commented Feb 22, 2019

flash1293 commented Feb 22, 2019

elasticmachine commented Feb 22, 2019

elasticmachine commented Feb 22, 2019

flash1293 commented Feb 24, 2019

elasticmachine commented Feb 24, 2019

ppisljar Feb 25, 2019

flash1293 Feb 25, 2019

ppisljar Feb 25, 2019

flash1293 Feb 25, 2019

elasticmachine commented Feb 25, 2019

ppisljar left a comment

Fix pushed down chart on multi line horizontal legends #31466

Fix pushed down chart on multi line horizontal legends #31466

Conversation

flash1293 commented Feb 19, 2019 • edited by timroes Loading

Summary

Checklist

For maintainers

elasticmachine commented Feb 19, 2019

💔 Build Failed

elasticmachine commented Feb 19, 2019

💔 Build Failed

elasticmachine commented Feb 19, 2019

💔 Build Failed

elasticmachine commented Feb 19, 2019

💚 Build Succeeded

ppisljar Feb 21, 2019

Choose a reason for hiding this comment

flash1293 Feb 21, 2019

Choose a reason for hiding this comment

ppisljar Feb 21, 2019

Choose a reason for hiding this comment

flash1293 Feb 21, 2019 • edited Loading

Choose a reason for hiding this comment

elasticmachine commented Feb 21, 2019

💔 Build Failed

flash1293 commented Feb 22, 2019

elasticmachine commented Feb 22, 2019

💔 Build Failed

elasticmachine commented Feb 22, 2019

💔 Build Failed

flash1293 commented Feb 22, 2019

elasticmachine commented Feb 22, 2019

💔 Build Failed

elasticmachine commented Feb 22, 2019

💚 Build Succeeded

flash1293 commented Feb 24, 2019

elasticmachine commented Feb 24, 2019

💚 Build Succeeded

ppisljar Feb 25, 2019

Choose a reason for hiding this comment

flash1293 Feb 25, 2019

Choose a reason for hiding this comment

ppisljar Feb 25, 2019

Choose a reason for hiding this comment

flash1293 Feb 25, 2019

Choose a reason for hiding this comment

elasticmachine commented Feb 25, 2019

💚 Build Succeeded

ppisljar left a comment

Choose a reason for hiding this comment

flash1293 commented Feb 19, 2019 •

edited by timroes

Loading

flash1293 Feb 21, 2019 •

edited

Loading