Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

React/EUI-ify indexed fields table #16695

Merged
merged 16 commits into from
Mar 9, 2018
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -190,6 +190,7 @@
"redux-thunk": "2.2.0",
"regression": "2.0.0",
"request": "2.61.0",
"reselect": "^3.0.1",
"resize-observer-polyfill": "1.2.1",
"rimraf": "2.4.3",
"rison-node": "1.0.0",
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -137,10 +137,7 @@

<!-- Tab content -->
<div class="kuiVerticalRhythm">
<indexed-fields-table
ng-show="state.tab == 'indexedFields'"
class="fields indexed-fields"
></indexed-fields-table>
<div id="reactIndexedFieldsTable"></div>

<div id="reactScriptedFieldsTable"></div>

Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,5 @@
import _ from 'lodash';
import './index_header';
import './indexed_fields_table';
import './scripted_field_editor';
import './source_filters_table';
import { KbnUrlProvider } from 'ui/url';
Expand All @@ -9,11 +8,14 @@ import { fatalError } from 'ui/notify';
import uiRoutes from 'ui/routes';
import { uiModules } from 'ui/modules';
import template from './edit_index_pattern.html';
import { FieldWildcardProvider } from 'ui/field_wildcard';

import React from 'react';
import { render, unmountComponentAtNode } from 'react-dom';
import { IndexedFieldsTable } from './indexed_fields_table';
import { ScriptedFieldsTable } from './scripted_fields_table';

const REACT_INDEXED_FIELDS_DOM_ELEMENT_ID = 'reactIndexedFieldsTable';
Copy link
Contributor

@chrisronline chrisronline Mar 8, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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,

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks, implemented similar changes

const REACT_SCRIPTED_FIELDS_DOM_ELEMENT_ID = 'reactScriptedFieldsTable';

function updateScriptedFieldsTable($scope, $state) {
Expand Down Expand Up @@ -54,6 +56,41 @@ function destroyScriptedFieldsTable() {
node && unmountComponentAtNode(node);
}

function updateIndexedFieldsTable($scope, $state) {
if ($state.tab === 'indexedFields') {
$scope.$$postDigest(() => {
const node = document.getElementById(REACT_INDEXED_FIELDS_DOM_ELEMENT_ID);
if (!node) {
return;
}

render(
<IndexedFieldsTable
fields={$scope.fields}
indexPattern={$scope.indexPattern}
fieldFilter={$scope.fieldFilter}
fieldWildcardMatcher={$scope.fieldWildcardMatcher}
indexedFieldTypeFilter={$scope.indexedFieldTypeFilter}
helpers={{
redirectToRoute: (obj, route) => {
$scope.kbnUrl.redirectToRoute(obj, route);
$scope.$apply();
},
}}
/>,
node,
);
});
} else {
destroyIndexedFieldsTable();
}
}

function destroyIndexedFieldsTable() {
const node = document.getElementById(REACT_INDEXED_FIELDS_DOM_ELEMENT_ID);
node && unmountComponentAtNode(node);
}

uiRoutes
.when('/management/kibana/indices/:indexPatternId', {
template,
Expand Down Expand Up @@ -87,7 +124,9 @@ uiModules.get('apps/management')
$scope, $location, $route, config, courier, Notifier, Private, AppState, docTitle, confirmModal) {
const notify = new Notifier();
const $state = $scope.state = new AppState();
const { fieldWildcardMatcher } = Private(FieldWildcardProvider);

$scope.fieldWildcardMatcher = fieldWildcardMatcher;
$scope.editSectionsProvider = Private(IndicesEditSectionsProvider);
$scope.kbnUrl = Private(KbnUrlProvider);
$scope.indexPattern = $route.current.locals.indexPattern;
Expand All @@ -100,6 +139,9 @@ 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);
});

$scope.refreshFilters = function () {
Expand All @@ -123,6 +165,7 @@ uiModules.get('apps/management')

$scope.changeTab = function (obj) {
$state.tab = obj.index;
updateIndexedFieldsTable($scope, $state);
updateScriptedFieldsTable($scope, $state);
$state.save();
};
Expand All @@ -139,7 +182,10 @@ 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();
},
title: 'Refresh field list?'
};
confirmModal(
Expand Down Expand Up @@ -187,8 +233,21 @@ uiModules.get('apps/management')
};

$scope.$watch('fieldFilter', () => {
if ($scope.fieldFilter !== undefined && $state.tab === 'scriptedFields') {
updateScriptedFieldsTable($scope, $state);
if ($scope.fieldFilter === undefined) {
return;
}

switch($state.tab) {
case 'indexedFields':
updateIndexedFieldsTable($scope, $state);
case 'scriptedFields':
updateScriptedFieldsTable($scope, $state);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
}

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated 👍

});

$scope.$watch('indexedFieldTypeFilter', () => {
if ($scope.indexedFieldTypeFilter !== undefined && $state.tab === 'indexedFields') {
updateIndexedFieldsTable($scope, $state);
}
});

Expand All @@ -199,8 +258,7 @@ uiModules.get('apps/management')
});

$scope.$on('$destory', () => {
destroyIndexedFieldsTable();
destroyScriptedFieldsTable();
});

updateScriptedFieldsTable($scope, $state);
});
Original file line number Diff line number Diff line change
@@ -0,0 +1,97 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`IndexedFieldsTable should filter based on the query bar 1`] = `
<div>
<Table
editField={[Function]}
indexPattern={
Object {
"getNonScriptedFields": [Function],
}
}
items={
Array [
Object {
"displayName": "Elastic",
"excluded": false,
"format": undefined,
"indexPattern": undefined,
"name": "Elastic",
"routes": undefined,
"searchable": true,
},
]
}
/>
</div>
`;

exports[`IndexedFieldsTable should filter based on the type filter 1`] = `
<div>
<Table
editField={[Function]}
indexPattern={
Object {
"getNonScriptedFields": [Function],
}
}
items={
Array [
Object {
"displayName": "timestamp",
"excluded": false,
"format": undefined,
"indexPattern": undefined,
"name": "timestamp",
"routes": undefined,
"type": "date",
},
]
}
/>
</div>
`;

exports[`IndexedFieldsTable should render normally 1`] = `
<div>
<Table
editField={[Function]}
indexPattern={
Object {
"getNonScriptedFields": [Function],
}
}
items={
Array [
Object {
"displayName": "Elastic",
"excluded": false,
"format": undefined,
"indexPattern": undefined,
"name": "Elastic",
"routes": undefined,
"searchable": true,
},
Object {
"displayName": "timestamp",
"excluded": false,
"format": undefined,
"indexPattern": undefined,
"name": "timestamp",
"routes": undefined,
"type": "date",
},
Object {
"displayName": "conflictingField",
"excluded": false,
"format": undefined,
"indexPattern": undefined,
"name": "conflictingField",
"routes": undefined,
"type": "conflict",
},
]
}
/>
</div>
`;
Original file line number Diff line number Diff line change
@@ -0,0 +1,96 @@
import React from 'react';
import { shallow } from 'enzyme';

import { IndexedFieldsTable } from '../indexed_fields_table';

jest.mock('@elastic/eui', () => ({
EuiFlexGroup: 'eui-flex-group',
EuiFlexItem: 'eui-flex-item',
EuiIcon: 'eui-icon',
EuiInMemoryTable: 'eui-in-memory-table',
TooltipTrigger: 'tooltip-trigger'
}));

jest.mock('../components/table', () => ({
// Note: this seems to fix React complaining about non lowercase attributes
Table: () => {
return 'table';
}
}));

const helpers = {
redirectToRoute: () => {},
};

const fields = [
{ name: 'Elastic', displayName: 'Elastic', searchable: true },
{ name: 'timestamp', displayName: 'timestamp', type: 'date' },
{ name: 'conflictingField', displayName: 'conflictingField', type: 'conflict' },
];

const indexPattern = {
getNonScriptedFields: () => fields,
};

describe('IndexedFieldsTable', () => {
it('should render normally', async () => {
const component = shallow(
<IndexedFieldsTable
fields={fields}
indexPattern={indexPattern}
helpers={helpers}
fieldWildcardMatcher={() => {}}
/>
);

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

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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();

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so much better!!

await component.update(); // Fire `componentWillMount()`
await component.update(); // Force update the component post async actions

expect(component).toMatchSnapshot();
});

it('should filter based on the query bar', async () => {
const component = shallow(
<IndexedFieldsTable
fields={fields}
indexPattern={indexPattern}
helpers={helpers}
fieldWildcardMatcher={() => {}}
/>
);

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

component.setProps({ fieldFilter: 'Elast' });
component.update();

expect(component).toMatchSnapshot();
});

it('should filter based on the type filter', async () => {
const component = shallow(
<IndexedFieldsTable
fields={fields}
indexPattern={indexPattern}
helpers={helpers}
fieldWildcardMatcher={() => {}}
/>
);

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

component.setProps({ indexedFieldTypeFilter: 'date' });
component.update();

expect(component).toMatchSnapshot();
});
});
Loading