[Vis Editor] EUIfication of agg and agg-group directives by maryia-lapata · Pull Request #40866 · elastic/kibana

maryia-lapata · 2019-07-11T14:13:43Z

Summary

EUIfication of vis-editor-agg and vis-editor-agg-group directives in Default Editor, Data tab.
Part of #30922.

To bind React controls with AngularJS form and to have a proper validation flow, previously ngModel was created for each DefaultEditorAggParams component. This approach is left except that ngModel is created for each DefaultEditorAggGroup component only. Since we need that AngularJS form and React controls still work together, we cannot get rid of ngModel-approach until the main form (visualizeEditor) is migrated to React. Before that we need to migrate option tabs (#38273).

This PR also includes:

Getting indexPattern from agg(agg.getIndexPattern()) instead of from vis(vis.indexPattern) thereby getting rid of vis prop in DefaultEditorAgg;
Removal of draggable-* directives and dragula dependency since it isn't used anymore. For drag- and-drop functionality Eui components are used (EuiDragDropContext , EuiDroppable, EuiDraggable);
Removal of React component wrappers agg_add.js, agg_params.js, agg_controls.js;
Removal of keyboardMove directive;
TS for aggGroupNameMaps;
Removal of unused styles;

This PR also fixes #42054.

Screenshots:

Collapsed aggs:

Agg

Error

Note:

Previously during dragging aggs, an opened agg accordion was collapsed at the moment of drag-and-drop, and agg accordion returned to the state before dragging started (opened/collapsed).

Now an opened agg accordion isn't collapsed at the moment of dragging since EUIAccordion's closed/opened state isn't managed by code (elastic/eui#2130), just by clicking on the caret icon. So a user has to collapse an agg before dragging for convenience.

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

Dev-Docs

`Schema.deprecate` removed

The deprecate boolean flag on Schema when declaring schemas for visualizations has been removed.

maryia-lapata · 2019-07-15T08:57:48Z

For this PR EUIAccordion is used inside of other EUIAccordion. And there is an issue with parent accordion height calculation:

When Y-axis accordion calculates the height, it sees the old height of Advanced accordion.

@chandlerprall maybe you have some ideas regarding how to fix that issue?

maryia-lapata · 2019-07-25T07:54:57Z

@cchaos is the title size (Metricks, Buckets) ok?

I'm asking because of the comment in Optional Pie tab (#41901 (comment)). I think that titles on the Data tab and titles on optional tabs should be the same.

elasticmachine · 2019-07-25T08:33:14Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

cchaos · 2019-07-25T13:15:15Z

@maryia-lapata I agree, they should be the same. I didn't realize they changed here too. Go ahead and make sure they're all EuiTitle size="xs"

maryia-lapata · 2019-07-25T13:29:25Z

I agree, they should be the same. I didn't realize they changed here too. Go ahead and make sure they're all EuiTitle size="xs"

@cchaos thanks.
The PR and screenshots was updated.

elasticmachine · 2019-07-26T08:42:25Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

timroes · 2019-07-26T09:10:01Z

package.json

    "d3": "3.5.17",
    "d3-cloud": "1.2.5",
    "del": "^4.0.0",
-    "dragula": "3.7.2",


timroes · 2019-07-26T10:41:10Z

src/legacy/ui/public/vis/editors/default/agg_group.js

+          value => {
+            $scope.formIsTouched = value;
+          },
+          true


This should be a boolean value (ngModelCtrl.$touched), so it does not really make sense making a deep object comparison here, we could just get rid of the true I think.

You're right. Fixed.

timroes · 2019-07-26T11:37:12Z

src/legacy/ui/public/vis/editors/default/components/default_editor_agg_group.tsx

+
+  const [aggsState, setAggsState] = useReducer(aggGroupReducer, group, initAggsState);
+
+  const isGroupValid = Object.entries(aggsState).every(([, item]) => item.valid);


Let's use Object.values if we're only planning to use the value and not the key.

timroes · 2019-07-26T11:56:49Z

src/legacy/ui/public/vis/editors/default/components/default_editor_agg_group.tsx

+    setValidity(isGroupValid);
+  }, [isGroupValid]);
+
+  const onDragEnd = ({ source, destination }: any) => {


any ... 😔

I'll open an issue for EUI to properly export those types. Let's for now at least simply type { source: { index: number }; destination: {index: number} } as a local type in this file.

elastic/eui#2172

Thank you.
I added a local type, but there is still an issue with TS, so I disabled TS for one line with comment.

chandlerprall · 2019-07-26T15:18:17Z

src/legacy/ui/public/vis/editors/default/components/default_editor_agg_group.tsx

+    result: { index: number };
+    provided: { index: number };
+  }
+  const onDragEnd = ({ result, provided }: DragDropProps) => {


This shouldn't be functioning,

result and provided are separate arguments i.e. (result, provided) instead of ({ result, provided })

result doesn't have an index property on it, instead there is source.index and destination.index (destination could also be null or undefined).

provided has the shape:

{ announce: (message: string) => void }

Getting the correct shapes here should remove the need for the @ts-ignore below

That's my bad. Thank you for noticing and explanation.

elasticmachine · 2019-07-29T07:44:41Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

timroes

Tested on Chrome Linux, seems to work fine. Code LGTM.

elasticmachine · 2019-07-29T10:25:59Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

…2126) * Create default_editor_agg.tsx * Create default_editor_agg_group * Apply drag and drop * Remove unused dragula dependency * Remove old mocha tests * Add ts for state * Update functional tests * Update touched condition * Apply styles for accordion button content * Apply truncate for agg description * Remove unused styles * Separate common props * Move aggGroupNamesMap to agg_group.js * Update _sidebar.scss * Pass schemas prop * Prevent scroll bar and add space * Remove unused min from stats * Add OnAggParamsChange type * Show error as an icon * Update background color * Update title size * Remove Schema.deprecate since it's not used

maryia-lapata added 7 commits July 8, 2019 14:23

Create default_editor_agg.tsx

bc31967

Create default_editor_agg_group

9f4f5df

Apply drag and drop

e9a2403

Remove unused files

e6e6559

Remove unused dragula dependency

d5cf2c9

Merge branch 'master' into vis-editor-agg

7ce703c

Refactoring

492a339

maryia-lapata added the release_note:skip Skip the PR/issue when compiling release notes label Jul 11, 2019

This comment has been minimized.

Sign in to view

maryia-lapata added 2 commits July 12, 2019 10:16

Merge branch 'master' into vis-editor-agg

2458e9d

Remove old mocha tests

8120251

This comment has been minimized.

Sign in to view

Add ts for state

7713f2d

This comment has been minimized.

Sign in to view

Update functional tests

17b3b54

This comment has been minimized.

Sign in to view

maryia-lapata added 2 commits July 15, 2019 11:14

Update touched condition

12b1882

Merge branch 'master' into vis-editor-agg

1df5189

maryia-lapata requested a review from sulemanof July 15, 2019 08:16

maryia-lapata added 2 commits July 15, 2019 12:23

Update visualize_page.js

bc3ef56

Move touched logic to function

08f8f22

This comment has been minimized.

Sign in to view

maryia-lapata added 2 commits July 15, 2019 16:37

Apply styles for accordion button content

32b0d05

Remove class

faa5c68

This comment has been minimized.

Sign in to view

Apply truncate for agg description

ded8708

This comment has been minimized.

Sign in to view

maryia-lapata added 2 commits July 16, 2019 11:56

Merge branch 'master' into vis-editor-agg

6ffca2e

Fix merge conflict

54d7998

maryia-lapata added 2 commits July 25, 2019 10:35

Update background color

ed2426f

Merge branch 'master' into vis-editor-agg

b1bfa53

Update title size

20ee812

cchaos approved these changes Jul 25, 2019

View reviewed changes

This comment has been minimized.

Sign in to view

Merge branch 'master' into vis-editor-agg

5dd42db

timroes suggested changes Jul 26, 2019

View reviewed changes

maryia-lapata added 3 commits July 26, 2019 16:26

Fix code review comments

187fa64

Merge branch 'master' into vis-editor-agg

d290f6a

FIx TS

56b631d

This comment has been minimized.

Sign in to view

chandlerprall reviewed Jul 26, 2019

View reviewed changes

maryia-lapata added 2 commits July 29, 2019 09:26

Merge branch 'master' into vis-editor-agg

de12321

Update TS

c228d8d

Remove Schema.deprecate since it's not used

2346bc0

timroes added the release_note:plugin_api_changes Contains a Plugin API changes section for the breaking plugin API changes section. label Jul 29, 2019

timroes approved these changes Jul 29, 2019

View reviewed changes

maryia-lapata merged commit 85f9ef6 into elastic:master Jul 29, 2019

maryia-lapata mentioned this pull request Jul 29, 2019

[7.x] [Vis Editor] EUIfication of agg and agg-group directives (#40866) #42126

Merged

maryia-lapata added the backported label Jul 29, 2019

sulemanof mentioned this pull request Oct 18, 2019

Visual Bug while Dragging Aggregations in Visualization Editor #26330

Closed


		const [aggsState, setAggsState] = useReducer(aggGroupReducer, group, initAggsState);

		const isGroupValid = Object.entries(aggsState).every(([, item]) => item.valid);

Conversation

maryia-lapata commented Jul 11, 2019 • edited by timroes Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Screenshots:

Note:

Checklist

For maintainers

Dev-Docs

Schema.deprecate removed

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

maryia-lapata commented Jul 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

maryia-lapata commented Jul 25, 2019

Uh oh!

elasticmachine commented Jul 25, 2019

💚 Build Succeeded

Uh oh!

cchaos commented Jul 25, 2019

Uh oh!

maryia-lapata commented Jul 25, 2019

Uh oh!

This comment has been minimized.

elasticmachine commented Jul 26, 2019

💚 Build Succeeded

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

This comment has been minimized.

This comment has been minimized.

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Jul 29, 2019

💚 Build Succeeded

Uh oh!

timroes left a comment

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Jul 29, 2019

💚 Build Succeeded

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

maryia-lapata commented Jul 11, 2019 •

edited by timroes

Loading

`Schema.deprecate` removed

maryia-lapata commented Jul 15, 2019 •

edited

Loading