React/EUI-ify indexed fields table #16695

jen-huang · 2018-02-13T06:22:31Z

Fixes #16694
Relates to #15721

Converts the indexed fields table to React/EUI.

chrisronline

Awesome! This looks great! The code pattern is probably the best I've ever seen ;)

I did find a few things:

Tooltip positioning

I'm not sure if this is fixable outside of EUI but this is definitely misaligned.

Yes indicators

I'm not sure I love this choice of icon to indicate yes. Did this come down from design?

Refresh fields

This doesn't seem to work for me. If I click the button, the fields are fetched, but the table isn't updated. You probably just need to re render the table once this happens in the angular code

jen-huang · 2018-02-14T00:52:09Z

@chrisronline Thanks for the review!

Issue created for EUI: Fix Tooltip positioning eui#401
Yes, design is here. I left out the grey circle indicators for no after speaking to @snide about legibility.
Can you give me an example of when the contents of this table would change after refreshing the fields? I have a potential fix that triggers re-rendering, but having a hard time finding a use case where the info is updated, to verify my fix.

cjcenizal

Looks great overall! Just had a couple of minor suggestions.

I have to say that I am not really feeling the dots in the table. 😐 My brain just doesn't pick up their meaning as quickly as with checkmarks. I also find myself feeling a bit unsure about what I can do with the table when the actions are hidden until you hover. I didn't realize I could edit the rows until I accidentally stumbled upon that functionality.

cjcenizal · 2018-02-15T00:39:37Z

...e_plugins/kibana/public/management/sections/indices/edit_index_pattern/edit_index_pattern.js

+        } else if($state.tab === 'scriptedFields') {
+          updateScriptedFieldsTable($scope, $state);
+        }
+      }


Minor suggestion: how about early exiting here to avoid nesting, and a slightly terser switch instead of else if?

if ($scope.fieldFilter === undefined) { return; } switch ($state.tab) { case 'indexedFields': updateIndexedFieldsTable($scope, $state); case 'scriptedFields': updateScriptedFieldsTable($scope, $state); }

Updated 👍

cjcenizal · 2018-02-15T00:42:21Z

...anagement/sections/indices/edit_index_pattern/indexed_fields_table/components/table/table.js

+}
+
+
+// import React, { Component } from 'react';


Can we delete this commented out code?

Wow how did that get in there... 🙈

jen-huang · 2018-02-16T16:58:43Z

@cjcenizal Thanks for reviewing! Might the hidden actions be something that can be added to TableOfRecords? Some property like show: true; so that user doesn't have to hover to see the actions column?

@snide Any thoughts on CJ's comments above about dots vs checkmarks?

elasticmachine · 2018-02-16T17:25:58Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

cjcenizal

💰LGTM!

elasticmachine · 2018-02-17T00:01:04Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-02-17T03:07:07Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-02-28T22:54:57Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-03-01T00:49:45Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

chrisronline · 2018-03-05T00:11:49Z

@jen-huang I don't think it's a requirement, but feel free to check out the new table implementation in EUI and how @cjcenizal updated the scripted fields table to use it #16901. It might be cool to start using that in this PR

elasticmachine · 2018-03-06T22:26:41Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

…dency to 0.17

elasticmachine · 2018-03-07T18:41:50Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

jen-huang · 2018-03-07T18:44:40Z

@chrisronline PR ready for another pass with EuiTableInMemory. Pulled in reselect as well to match #16649.

chrisronline · 2018-03-08T00:58:46Z

...e_plugins/kibana/public/management/sections/indices/edit_index_pattern/edit_index_pattern.js

 import { ScriptedFieldsTable } from './scripted_fields_table';

+const REACT_INDEXED_FIELDS_DOM_ELEMENT_ID = 'reactIndexedFieldsTable';


I'm still seeing an issue where the fields aren't refreshing once I click Refresh Fields. I think it's because this change is never propagating to the React component.

I'm wondering if we just let this parent container control the fields and pass it in so we can easily tie a change in the fields to the React lifecycle. Or maybe find another way.

But if you like the first idea, here is a some code I whipped up that might work:

diff --git a/src/core_plugins/kibana/public/management/sections/indices/edit_index_pattern/edit_index_pattern.js b/src/core_plugins/kibana/public/management/sections/indices/edit_index_pattern/edit_index_pattern.js index f1187c3fed..02ab1c6540 100644 --- a/src/core_plugins/kibana/public/management/sections/indices/edit_index_pattern/edit_index_pattern.js +++ b/src/core_plugins/kibana/public/management/sections/indices/edit_index_pattern/edit_index_pattern.js @@ -67,6 +67,7 @@ function updateIndexedFieldsTable($scope, $state) { render( <IndexedFieldsTable indexPattern={$scope.indexPattern} + fields={$scope.fields} fieldFilter={$scope.fieldFilter} fieldWildcardMatcher={$scope.fieldWildcardMatcher} indexedFieldTypeFilter={$scope.indexedFieldTypeFilter} @@ -138,6 +139,7 @@ uiModules.get('apps/management') $scope.$watch('indexPattern.fields', function () { $scope.editSections = $scope.editSectionsProvider($scope.indexPattern); $scope.refreshFilters(); + $scope.fields = $scope.indexPattern.getNonScriptedFields(); updateIndexedFieldsTable($scope, $state); updateScriptedFieldsTable($scope, $state); }); @@ -180,7 +182,11 @@ uiModules.get('apps/management') $scope.refreshFields = function () { const confirmModalOptions = { confirmButtonText: 'Refresh', - onConfirm: () => { $scope.indexPattern.refreshFields(); }, + onConfirm: async () => { + await $scope.indexPattern.refreshFields(); + $scope.fields = $scope.indexPattern.getNonScriptedFields(); + updateIndexedFieldsTable($scope, $state); + }, title: 'Refresh field list?' }; confirmModal( diff --git a/src/core_plugins/kibana/public/management/sections/indices/edit_index_pattern/indexed_fields_table/indexed_fields_table.js b/src/core_plugins/kibana/public/management/sections/indices/edit_index_pattern/indexed_fields_table/indexed_fields_table.js index 995f4bbb37..acea91db5d 100644 --- a/src/core_plugins/kibana/public/management/sections/indices/edit_index_pattern/indexed_fields_table/indexed_fields_table.js +++ b/src/core_plugins/kibana/public/management/sections/indices/edit_index_pattern/indexed_fields_table/indexed_fields_table.js @@ -10,6 +10,7 @@ import { export class IndexedFieldsTable extends Component { static propTypes = { indexPattern: PropTypes.object.isRequired, + fields: PropTypes.array.isRequired, fieldFilter: PropTypes.string, indexedFieldTypeFilter: PropTypes.string, helpers: PropTypes.shape({ @@ -22,12 +23,14 @@ export class IndexedFieldsTable extends Component { super(props); this.state = { - fields: [] + fields: this.mapFields(this.props.fields), }; } - componentWillMount() { - this.fetchFields(); + componentWillReceiveProps(nextProps) { + if (nextProps.fields !== this.props.fields) { + this.setState({ fields: this.mapFields(nextProps.fields) }); + } } mapFields(fields) { @@ -47,13 +50,6 @@ export class IndexedFieldsTable extends Component { }); } - fetchFields = async () => { - const fields = this.mapFields(await this.props.indexPattern.getNonScriptedFields()); - this.setState({ - fields - }); - } - getFilteredFields = createSelector( (state) => state.fields, (state, props) => props.fieldFilter,

thanks, implemented similar changes

bmcconaghy

a couple of minor things, but looking good.

bmcconaghy · 2018-02-16T15:30:59Z

.../management/sections/indices/edit_index_pattern/indexed_fields_table/indexed_fields_table.js

+
+  mapFields(fields) {
+    const { indexPattern, fieldWildcardMatcher } = this.props;
+    const fieldWildcardMatch = fieldWildcardMatcher((indexPattern.sourceFilters && indexPattern.sourceFilters.map(f => f.value) || []));


this line is really long

broke it up

bmcconaghy · 2018-03-08T13:26:52Z

.../management/sections/indices/edit_index_pattern/indexed_fields_table/indexed_fields_table.js

+          ...field,
+          displayName: field.displayName,
+          routes: field.routes,
+          indexPattern: field.indexPattern,


lines 41-43 are covered by line 40.

displayName, routes, and indexPattern aren't enumerable properties :(

Object.keys(field) (7) ["name", "type", "count", "scripted", "searchable", "aggregatable", "readFromDocValues"]

ahh, nm then :-)

Can we leave a comment here explaining that? I'm pretty sure I would come along and make the same assumption Bill did. :)

jen-huang · 2018-03-08T18:17:46Z

@chrisronline Ready for another pass, refresh fields should work now.

elasticmachine · 2018-03-08T19:00:51Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

chrisronline · 2018-03-08T19:31:37Z

...tions/indices/edit_index_pattern/indexed_fields_table/__tests__/indexed_fields_table.test.js

+
+    // Allow the componentWillMount code to execute
+    // https://github.com/airbnb/enzyme/issues/450
+    await component.update(); // Fire `componentWillMount()`


We can simplify all of this from #16968

// Ensure all promises resolve await new Promise(resolve => process.nextTick(resolve)); // Ensure the state changes are reflected component.update();

so much better!!

chrisronline

This is looking GREAT! I just have one minor request for the tests that should hopefully make them more clear.

jen-huang · 2018-03-08T23:48:06Z

@chrisronline Updated!

elasticmachine · 2018-03-09T00:31:22Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

chrisronline

LGTM!!!

* React/EUI-ifying indexed fields table

jen-huang requested review from chrisronline, cjcenizal and bmcconaghy February 13, 2018 06:22

chrisronline mentioned this pull request Feb 13, 2018

[META] EUIfication #15721

Closed

47 tasks

jen-huang added :Management v7.0.0 v6.3.0 review labels Feb 13, 2018

jen-huang self-assigned this Feb 13, 2018

chrisronline suggested changes Feb 13, 2018

View reviewed changes

cjcenizal suggested changes Feb 15, 2018

View reviewed changes

cjcenizal approved these changes Feb 16, 2018

View reviewed changes

elastic deleted a comment from elasticmachine Feb 17, 2018

jen-huang force-pushed the eui/indexed-fields-table branch from c3d462c to 8caf7e2 Compare March 6, 2018 21:43

jen-huang added 7 commits March 7, 2018 09:54

First pass at React/EUI-ifying indexed fields table, update eui depen…

45afdf7

…dency to 0.17

Add back field type/name filtering

20fe3b8

Match scripted fields conversion

3109f61

Finish functionality

ef63d0a

Add tests

6a43eb8

Update tables when fields are refreshed

cd2a2c1

Update PR

22d18d8

jen-huang added 7 commits March 7, 2018 09:54

Fix tests

35021ba

Fix functional tests

898f99b

Fix more tests

adb450b

Update snapshots

ecd1ec0

Replace EuiTableOfRecords with EuiInMemoryTable

a055a3a

Fix tests

c2d58bd

Use reselect to update table

979acef

jen-huang force-pushed the eui/indexed-fields-table branch from 8caf7e2 to 979acef Compare March 7, 2018 17:57

chrisronline reviewed Mar 8, 2018

View reviewed changes

bmcconaghy approved these changes Mar 8, 2018

View reviewed changes

Pass fields from parent to support refreshing fields, update tests

c5633aa

chrisronline reviewed Mar 8, 2018

View reviewed changes

chrisronline suggested changes Mar 8, 2018

View reviewed changes

Update tests

169e31f

chrisronline approved these changes Mar 9, 2018

View reviewed changes

jen-huang merged commit e755514 into elastic:master Mar 9, 2018

jen-huang mentioned this pull request Mar 9, 2018

[6.x] React/EUI-ify indexed fields table (#16695) #17068

Merged

jen-huang added a commit to jen-huang/kibana that referenced this pull request Mar 9, 2018

React/EUI-ify indexed fields table (elastic#16695)

5832f08

* React/EUI-ifying indexed fields table

jen-huang added a commit that referenced this pull request Mar 9, 2018

React/EUI-ify indexed fields table (#16695) (#17068)

03d9c42

* React/EUI-ifying indexed fields table

cjcenizal mentioned this pull request Mar 12, 2018

Componentization: List Index Pattern fields using Table component #11902

Closed

jen-huang deleted the eui/indexed-fields-table branch June 5, 2019 19:47

		import { ScriptedFieldsTable } from './scripted_fields_table';

		const REACT_INDEXED_FIELDS_DOM_ELEMENT_ID = 'reactIndexedFieldsTable';

React/EUI-ify indexed fields table #16695

React/EUI-ify indexed fields table #16695

Conversation

jen-huang commented Feb 13, 2018

chrisronline left a comment

Choose a reason for hiding this comment

Tooltip positioning

Yes indicators

Refresh fields

jen-huang commented Feb 14, 2018 • edited Loading

cjcenizal left a 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

jen-huang commented Feb 16, 2018

elasticmachine commented Feb 16, 2018

💔 Build Failed

cjcenizal left a comment • edited Loading

Choose a reason for hiding this comment

elasticmachine commented Feb 17, 2018

💔 Build Failed

elasticmachine commented Feb 17, 2018

💔 Build Failed

elasticmachine commented Feb 28, 2018

💔 Build Failed

elasticmachine commented Mar 1, 2018

💚 Build Succeeded

chrisronline commented Mar 5, 2018

elasticmachine commented Mar 6, 2018

💔 Build Failed

elasticmachine commented Mar 7, 2018

💚 Build Succeeded

jen-huang commented Mar 7, 2018

chrisronline Mar 8, 2018 • edited by jen-huang Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmcconaghy left a 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

jen-huang Mar 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jen-huang commented Mar 8, 2018

elasticmachine commented Mar 8, 2018

💚 Build Succeeded

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisronline left a comment

Choose a reason for hiding this comment

jen-huang commented Mar 8, 2018

elasticmachine commented Mar 9, 2018

💚 Build Succeeded

chrisronline left a comment

Choose a reason for hiding this comment

jen-huang commented Feb 14, 2018 •

edited

Loading

cjcenizal left a comment •

edited

Loading

chrisronline Mar 8, 2018 •

edited by jen-huang

Loading

jen-huang Mar 8, 2018 •

edited

Loading