Ensure custom set axis titles are preserved when loading a saved vis. #24176

lukeelmers · 2018-10-17T22:01:17Z

Summary

Previously, the visualization editor would lose a custom set axis title every
time you opened the editor and loaded a visualization or if you changed the
actual aggregation label. This was due to the way that agg labels were
overwriting custom axis titles.

This change simply checks to ensure the custom title hasn't already been
changed before overwriting it with an updated agg label.

Testing

Create a new point series vis with one y-axis agg.
In the editor Data tab, set a custom label "A" for your y-axis agg.
Go to the editor Metrics & Axes tab, and you will see the custom label "A" has been copied as the custom y-axis title.
Now change the custom y-axis title to "B" and click apply.
Note that changes display in the vis as expected.
Click save, exit the editor, go back to visualizations and re-open the vis.
Note that the custom y-axis title "B" is still displayed on the vis.
When navigating to the Metrics & Axes tab, you should continue to see the same title "B" (this is what was broken prior to this PR -- previously you would have seen "A" in the input field).

Checklist

~~[ ] 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~~

lukeelmers · 2018-10-17T22:02:43Z

src/core_plugins/kbn_vislib_vis_types/public/editors/__tests__/point_series.js

+    $parentScope.updateAxisTitle();
+    expect($parentScope.editorState.params.valueAxes[0].title.text).to.equal('Count');
+  });
+


Added the above 2 tests to validate existing editor behavior which was previously untested

lukeelmers · 2018-10-17T22:03:35Z

src/core_plugins/kbn_vislib_vis_types/public/editors/__tests__/point_series.js

+    $parentScope.editorState.aggs[0].params.customLabel = 'Custom Label';
+    $parentScope.updateAxisTitle();
+    expect($parentScope.editorState.params.valueAxes[0].title.text).to.equal('Custom Title');
+  });


This is the test that validates the actual change made in this PR

elasticmachine · 2018-10-17T23:42:34Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

lukeelmers · 2018-10-18T02:16:47Z

Jenkins, test this

elasticmachine · 2018-10-18T04:07:30Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

timroes

I am just a bit worried about the tests. They currently (and most likely already have beforehand) relied on a specific execution order, even though they are unit tests. Do you think we could modify the tests, to actually setup the scope from scratch for every test into the state it needs to be?

markov00

Code LGTM. Tested on OSX/Chrome.
As Tim suggested it would be also useful to have a functional test, where the test follow the exact path you have described on the PR description, involving saving of the visualization going back to the visualization list and back again to the visualization.

markov00 · 2018-10-18T08:20:12Z

retest

elasticmachine · 2018-10-18T09:57:47Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

lukeelmers · 2018-11-06T21:20:25Z

retest

elasticmachine · 2018-11-06T22:59:01Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

markov00 · 2018-11-07T09:47:54Z

@lukeelmers can you merge master? I've already seen this CI error (https://kibana-ci.elastic.co/job/elastic+kibana+pull-request+multijob-x-pack/7590/console). Not sure if was the merging or the retesting that solves that problem

ppisljar · 2018-11-07T10:00:52Z

src/core_plugins/kbn_vislib_vis_types/public/controls/point_series/value_axes.js

@@ -158,8 +158,11 @@ module.directive('vislibValueAxes', function () {
            label = matchingSeries[0].makeLabel();
          }
          if (lastAxisTitles[axis.id] !== label && label !== '') {
+            // Only overwrite the custom title with the value axis label if it hasn't been changed
+            if (lastAxisTitles[axis.id] === axis.title.text) {


wouldn't we want to change the title automatically if the actual aggregation was changed, or a field ?

select sum over bytes field

go to axes, change title from sum of bytes to sum of bytes downloaded

go back to data tab, and change field to something else ... what would you expect in this case ?

what about if you completely changed aggregation ?

@ppisljar Good catch -- it feels to me the expected behavior in the cases you've described would be that the custom title gets cleared when the agg type or field changes. I will give it some thought and follow up in a bit.

lukeelmers · 2018-11-08T00:22:05Z

Okay, I think I finally got to some behavior that I feel makes sense while still solving the original issue:

When an axis title is set manually, it will always override the custom label set on the agg
Otherwise, the axis title will be set to whatever the custom label is on the agg
If no custom label is specified on the agg, the axis title will be set to the default for the agg (i.e. whatever aggConfig.makeLabel() returns)
If the agg type is changed, axis titles are reset to aggConfg.makeLabel() (this prevents the weird state @ppisljar describes above)

See below and notice how the axis title & legend change based on what is changed in the editor:

I've also rebased on the latest master so (hopefully) we will get a green build now. Updated tests to follow shortly.

The one item this doesn't address is resetting the axis title when the agg field changes. This issue affects master already... I'll see if I can look further into this, or perhaps we capture that edge case as a separate issue.

lukeelmers · 2018-11-08T00:56:50Z

src/core_plugins/kbn_vislib_vis_types/public/controls/point_series/value_axes.js

@@ -136,10 +136,11 @@ module.directive('vislibValueAxes', function () {
        }, 1);
      };

-      const lastAxisTitles = {};
+      const lastCustomLabels = {};


I changed some variable names to make this all a little easier to follow.

elasticmachine · 2018-11-08T02:41:35Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

ppisljar

LGTM, tested on chrome linux

lukeelmers · 2018-11-09T00:32:46Z

Updated unit tests, added functional tests, rebased on latest master. @timroes @markov00 let me know if you have any other feedback.

elasticmachine · 2018-11-09T02:49:22Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

…alization. Previously, the visualization editor would lose a custom set axis title every time you opened the editor and loaded a visualization or if you changed the actual aggregation label. This was due to the way that agg labels were overwriting custom axis titles. This change simply checks to ensure the custom title hasn't already been changed before overwriting it with an updated agg label.

elasticmachine · 2018-11-15T01:48:56Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

timroes · 2018-11-15T08:37:34Z

No further notes from my side, please feel free to merge and also backport to 6.x and 6.5

…elastic#24176)

…#24176) (#25732)

…#24176) (#25733)

lukeelmers commented Oct 17, 2018

View reviewed changes

lukeelmers added Team:Visualizations Visualization editors, elastic-charts and infrastructure review Feature:Vis Editor Visualization editor issues labels Oct 17, 2018

lukeelmers requested review from timroes and markov00 October 17, 2018 22:05

timroes reviewed Oct 18, 2018

View reviewed changes

markov00 reviewed Oct 18, 2018

View reviewed changes

timroes requested a review from ppisljar November 7, 2018 08:06

ppisljar reviewed Nov 7, 2018

View reviewed changes

lukeelmers force-pushed the fix-axis-title branch from 899957c to 1d4cfeb Compare November 8, 2018 00:21

lukeelmers commented Nov 8, 2018

View reviewed changes

ppisljar approved these changes Nov 8, 2018

View reviewed changes

lukeelmers force-pushed the fix-axis-title branch from 1d4cfeb to 9872708 Compare November 9, 2018 00:32

lukeelmers added v7.0.0 v6.4.4 and removed review labels Nov 9, 2018

lukeelmers added the v6.5.1 label Nov 14, 2018

lukeelmers added 5 commits November 14, 2018 16:38

Reset axis title when agg type changes.

f0596ee

Reset axis title when agg field is changed.

fd31e37

Update point series vis type unit tests.

20b5916

Add functional tests for axis title bug.

6e6d168

lukeelmers force-pushed the fix-axis-title branch from 9872708 to 6e6d168 Compare November 14, 2018 23:38

lukeelmers merged commit e75bcda into elastic:master Nov 15, 2018

lukeelmers deleted the fix-axis-title branch November 15, 2018 15:57

lukeelmers restored the fix-axis-title branch November 15, 2018 15:57

lukeelmers deleted the fix-axis-title branch November 15, 2018 15:58

lukeelmers mentioned this pull request Nov 15, 2018

[6.x] Ensure custom set axis titles are preserved when loading a saved vis. (#24176) #25732

Merged

lukeelmers added a commit to lukeelmers/kibana that referenced this pull request Nov 15, 2018

Ensure custom set axis titles are preserved when loading a saved vis. (…

44c7463

…elastic#24176)

lukeelmers mentioned this pull request Nov 15, 2018

[6.5] Ensure custom set axis titles are preserved when loading a saved vis. (#24176) #25733

Merged

lukeelmers added a commit to lukeelmers/kibana that referenced this pull request Nov 15, 2018

Ensure custom set axis titles are preserved when loading a saved vis. (…

9806bd1

…elastic#24176)

lukeelmers added v6.6.0 v6.5.2 and removed v6.4.4 v6.5.1 labels Nov 15, 2018

lukeelmers added a commit that referenced this pull request Nov 15, 2018

Ensure custom set axis titles are preserved when loading a saved vis. (…

12d0355

…#24176) (#25732)

lukeelmers added a commit that referenced this pull request Nov 20, 2018

Ensure custom set axis titles are preserved when loading a saved vis. (…

e57379a

…#24176) (#25733)

maryia-lapata mentioned this pull request Sep 2, 2019

[Vis: Default Editor] EUIficate 'Metrics & Axis' tab #43772

Merged

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensure custom set axis titles are preserved when loading a saved vis. #24176

Ensure custom set axis titles are preserved when loading a saved vis. #24176

lukeelmers commented Oct 17, 2018

lukeelmers Oct 17, 2018

lukeelmers Oct 17, 2018

elasticmachine commented Oct 17, 2018

lukeelmers commented Oct 18, 2018

elasticmachine commented Oct 18, 2018

timroes left a comment

markov00 left a comment

markov00 commented Oct 18, 2018

elasticmachine commented Oct 18, 2018

lukeelmers commented Nov 6, 2018

elasticmachine commented Nov 6, 2018

markov00 commented Nov 7, 2018

ppisljar Nov 7, 2018

lukeelmers Nov 7, 2018

lukeelmers commented Nov 8, 2018 •

edited

Loading

lukeelmers Nov 8, 2018

elasticmachine commented Nov 8, 2018

ppisljar left a comment

lukeelmers commented Nov 9, 2018

elasticmachine commented Nov 9, 2018

elasticmachine commented Nov 15, 2018

timroes commented Nov 15, 2018

Ensure custom set axis titles are preserved when loading a saved vis. #24176

Ensure custom set axis titles are preserved when loading a saved vis. #24176

Conversation

lukeelmers commented Oct 17, 2018

Summary

Testing

Checklist

For maintainers

lukeelmers Oct 17, 2018

Choose a reason for hiding this comment

lukeelmers Oct 17, 2018

Choose a reason for hiding this comment

elasticmachine commented Oct 17, 2018

💔 Build Failed

lukeelmers commented Oct 18, 2018

elasticmachine commented Oct 18, 2018

💔 Build Failed

timroes left a comment

Choose a reason for hiding this comment

markov00 left a comment

Choose a reason for hiding this comment

markov00 commented Oct 18, 2018

elasticmachine commented Oct 18, 2018

💔 Build Failed

lukeelmers commented Nov 6, 2018

elasticmachine commented Nov 6, 2018

💔 Build Failed

markov00 commented Nov 7, 2018

ppisljar Nov 7, 2018

Choose a reason for hiding this comment

lukeelmers Nov 7, 2018

Choose a reason for hiding this comment

lukeelmers commented Nov 8, 2018 • edited Loading

lukeelmers Nov 8, 2018

Choose a reason for hiding this comment

elasticmachine commented Nov 8, 2018

💚 Build Succeeded

ppisljar left a comment

Choose a reason for hiding this comment

lukeelmers commented Nov 9, 2018

elasticmachine commented Nov 9, 2018

💚 Build Succeeded

elasticmachine commented Nov 15, 2018

💚 Build Succeeded

timroes commented Nov 15, 2018

lukeelmers commented Nov 8, 2018 •

edited

Loading