[Vis: Default editor] EUIficate Sub agg control #37979

maryia-lapata · 2019-06-04T12:02:08Z

Summary

EUIfication of Sub agg control for aggregation parameter in Default Editor, Data tab.
Part of #30922.

Steps to reproduce: create a visualization, choose an aggregation from Parent Pipeline Aggregations group (e.g. Derivative).

Details

validate-agg directive was removed
agg_select.js was updated. Now it has the same validation flow as agg_param.js has
functions isCompatibleAgg and safeMakeLabel were moved to a separate utils file (agg_utils.ts)

Screenshot:

Please pay your attention that validation a bit changed:

before: when a user chooses incompatible bucket aggregation, metrics aggregation input was in red
after: when a user chooses incompatible bucket aggregation, bucket aggregation input is in red

I changed this validation because it's a bit difficult to implement before case until agg_params.js will be migrated. Also I find it logical to make bucket aggregation input in red as that agg is incompatible. Reviewers, please, let me know if it's applicable.

Also now the error message is displayed underneath the agg form control:

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

cchaos · 2019-06-04T17:23:01Z

It doesn't seem to automatically put the sub-agg in an error state until you interact with it. Is there any way to force that state before you interact with it?

Also is there any way to change this message so that it is just help text underneath the sub-agg form control?

maryia-lapata · 2019-06-05T11:38:38Z

It doesn't seem to automatically put the sub-agg in an error state until you interact with it. Is there any way to force that state before you interact with it?

Oh yes, I updated the PR. Thank you for noticing.

Also is there any way to change this message so that it is just help text underneath the sub-agg form control?

I moved this message into agg selector's error section.
I updated a screenshot in the description, please take a look.

elasticmachine · 2019-06-05T12:07:12Z

Pinging @elastic/kibana-app

maryia-lapata · 2019-06-05T13:26:58Z

src/legacy/ui/public/agg_types/metrics/lib/parent_pipeline_agg_controller.js

@@ -79,9 +70,6 @@ const parentPipelineAggController = function ($scope) {

    // we aren't creating a custom aggConfig
    if (metricAgg !== 'custom') {
-      if (!$scope.state.aggs.find(agg => agg.id === metricAgg)) {


This logic was moved to MetricAggParamEditor component: https://github.com/elastic/kibana/pull/37979/files?file-filters%5B%5D=.tsx&owned-by%5B%5D=#diff-d09c7d350e3017c52e2e151fba97ef27R54

maryia-lapata · 2019-06-05T13:30:33Z

src/legacy/ui/public/vis/editors/default/agg_params.html

@@ -13,12 +13,6 @@
    style="display: none;">
 </div>

-<div ng-if="agg.error" class="form-group">
-  <p class="visEditorAggParam__error ng-binding">
-    {{agg.error}}


Now this error message is displayed in aggregation selector control: https://github.com/elastic/kibana/pull/37979/files?file-filters%5B%5D=.tsx&owned-by%5B%5D=#diff-9c2f5870ef0ffd3edbde4d2072e86430R98

Can you also change the exclamation point of the error message to just a period:

wylieconlon · 2019-06-07T17:25:49Z

I'm a bit confused about the warnings here. In both this branch and on master, I see the blue arrow indicating that a Derivative is valid without any X axis:

I see that this is not a regression, but it was confusing and seems related. What do you think?

sulemanof

Code and functionality LGTM! Tested locally.

maryia-lapata · 2019-06-10T11:27:18Z

I'm a bit confused about the warnings here. In both this branch and on master, I see the blue arrow indicating that a Derivative is valid without any X axis:

I see that this is not a regression, but it was confusing and seems related. What do you think?

@wylieconlon I tested on 6.7, the issue is reproduced. So for me it seems like a bug. I create an issue:
#38537

maryia-lapata · 2019-06-12T07:14:16Z

I believe that #38537 doesn't block migration. @wylieconlon could please continue with review?

elasticmachine · 2019-06-12T12:32:10Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

cchaos

LGTM, thx!

wylieconlon

LGTM

dmlemeshko

Changes to functional tests LGTM. Tested in both Chrome and Firefox

* EUIficate metric agg control * Fix translation errors * Display agg error underneath the last bucket agg form control * Update functional test * Update error message * Update parent_pipeline_agg_controller.js * Fix validation when metricAgg is invalid * Show error message when a filed is selected * Delete _terms_helper.tsx * Remove extra empty line * Update parent_pipeline_agg_helper.js * Update selector for test

* [Vis: Default editor] EUIficate Sub agg control (#37979) * EUIficate metric agg control * Fix translation errors * Display agg error underneath the last bucket agg form control * Update functional test * Update error message * Update parent_pipeline_agg_controller.js * Fix validation when metricAgg is invalid * Show error message when a filed is selected * Delete _terms_helper.tsx * Remove extra empty line * Update parent_pipeline_agg_helper.js * Update selector for test * Remove unused translation

maryia-lapata added 2 commits June 4, 2019 15:00

EUIficate metric agg control

0dd3dbf

Fix translation errors

f492e66

maryia-lapata changed the title ~~EUIficate metric agg control~~ [Vis: Default editor] EUIficate Metric agg control Jun 4, 2019

maryia-lapata changed the title ~~[Vis: Default editor] EUIficate Metric agg control~~ [Vis: Default editor] EUIficate Sub agg control Jun 4, 2019

Merge branch 'master' into vis-editor-sub-agg

2aa8de2

This comment has been minimized.

Sign in to view

maryia-lapata requested a review from cchaos June 4, 2019 14:49

maryia-lapata added 2 commits June 5, 2019 10:09

Merge branch 'master' into vis-editor-sub-agg

38ddd9a

Display agg error underneath the last bucket agg form control

b7a2257

This comment has been minimized.

Sign in to view

Update functional test

c2c31c2

maryia-lapata requested a review from sulemanof June 5, 2019 11:56

This comment has been minimized.

Sign in to view

maryia-lapata marked this pull request as ready for review June 5, 2019 12:06

maryia-lapata added Team:Visualizations Visualization editors, elastic-charts and infrastructure Feature:Vis Editor Visualization editor issues release_note:skip Skip the PR/issue when compiling release notes v7.3.0 v8.0.0 labels Jun 5, 2019

timroes mentioned this pull request Jun 5, 2019

EUIficate vis editor data panel #30922

Closed

29 tasks

maryia-lapata commented Jun 5, 2019

View reviewed changes

maryia-lapata added 2 commits June 6, 2019 11:07

Update error message

a858f9e

Merge branch 'master' into vis-editor-sub-agg

f8a58d3

This comment has been minimized.

Sign in to view

Merge branch 'master' into vis-editor-sub-agg

0bd4c5c

maryia-lapata added 4 commits June 7, 2019 14:44

Show error message when a filed is selected

0b1e3f3

Delete _terms_helper.tsx

55d4306

Remove extra empty line

e198965

Update parent_pipeline_agg_helper.js

48643b5

maryia-lapata requested review from wylieconlon, flash1293 and chrisdavies June 7, 2019 13:20

This comment has been minimized.

Sign in to view

Merge branch 'master' into vis-editor-sub-agg

9823bf3

This comment has been minimized.

Sign in to view

sulemanof approved these changes Jun 10, 2019

View reviewed changes

Merge branch 'master' into vis-editor-sub-agg

caba070

This comment has been minimized.

Sign in to view

maryia-lapata requested a review from dmlemeshko June 12, 2019 08:57

Update selector for test

117cff5

This comment has been minimized.

Sign in to view

Merge branch 'master' into vis-editor-sub-agg

59620d6

cchaos approved these changes Jun 12, 2019

View reviewed changes

wylieconlon approved these changes Jun 12, 2019

View reviewed changes

dmlemeshko approved these changes Jun 13, 2019

View reviewed changes

maryia-lapata merged commit 386619e into elastic:master Jun 13, 2019

maryia-lapata deleted the vis-editor-sub-agg branch June 13, 2019 08:18

maryia-lapata mentioned this pull request Jun 13, 2019

[7.x] [Vis: Default editor] EUIficate Sub agg control (#37979) #38866

Merged

maryia-lapata added the backported label Jun 13, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Vis: Default editor] EUIficate Sub agg control #37979

[Vis: Default editor] EUIficate Sub agg control #37979

maryia-lapata commented Jun 4, 2019 •

edited

Loading

This comment has been minimized.

cchaos commented Jun 4, 2019

This comment has been minimized.

maryia-lapata commented Jun 5, 2019

This comment has been minimized.

elasticmachine commented Jun 5, 2019

maryia-lapata Jun 5, 2019

maryia-lapata Jun 5, 2019

cchaos Jun 5, 2019

maryia-lapata Jun 6, 2019

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

wylieconlon commented Jun 7, 2019

This comment has been minimized.

sulemanof left a comment

maryia-lapata commented Jun 10, 2019

maryia-lapata commented Jun 12, 2019

This comment has been minimized.

This comment has been minimized.

elasticmachine commented Jun 12, 2019

cchaos left a comment

wylieconlon left a comment

dmlemeshko left a comment

[Vis: Default editor] EUIficate Sub agg control #37979

[Vis: Default editor] EUIficate Sub agg control #37979

Conversation

maryia-lapata commented Jun 4, 2019 • edited Loading

Summary

Details

Screenshot:

Checklist

For maintainers

This comment has been minimized.

cchaos commented Jun 4, 2019

This comment has been minimized.

maryia-lapata commented Jun 5, 2019

This comment has been minimized.

elasticmachine commented Jun 5, 2019

maryia-lapata Jun 5, 2019

Choose a reason for hiding this comment

maryia-lapata Jun 5, 2019

Choose a reason for hiding this comment

cchaos Jun 5, 2019

Choose a reason for hiding this comment

maryia-lapata Jun 6, 2019

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

wylieconlon commented Jun 7, 2019

This comment has been minimized.

sulemanof left a comment

Choose a reason for hiding this comment

maryia-lapata commented Jun 10, 2019

maryia-lapata commented Jun 12, 2019

This comment has been minimized.

This comment has been minimized.

elasticmachine commented Jun 12, 2019

💚 Build Succeeded

cchaos left a comment

Choose a reason for hiding this comment

wylieconlon left a comment

Choose a reason for hiding this comment

dmlemeshko left a comment

Choose a reason for hiding this comment

maryia-lapata commented Jun 4, 2019 •

edited

Loading