[Management] Source filters table in React #16649

chrisronline · 2018-02-09T18:36:51Z

Fixes #16540
Relates to #15721

This PR converts the source filters tables to using React/EUI.

cc @cjcenizal

elasticmachine · 2018-02-09T19:10:11Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-02-09T20:22:13Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

cjcenizal

It's amazing how quickly you've blazed through this! This looks great. I had a couple comments on the code and found a few UX quirks.

Disallow empty source filters

Can we disable the "Add" button until a non-empty string has been entered into the input field? I created some empty string source filters and they're basically useless.

Save on Enter

When I edited a source filter, it took me a few seconds to find the save button. How about we allow the user to save the source filter when they hit enter?

Also, I'm not sure it makes sense to allow you to delete a source filter while you're editing it. Can we replace the multi-action popover with a single "Save" EuiButtonEmpty when you're in editing mode?

Cancel editing mode

For that matter, how do you cancel editing mode? It would be nice if you could click a "Cancel" button and also hit ESC to exit editing mode.

Clicking save needs to close the popover

If you edit a source filter and then hit save, the popover remains open. Then if you click the "Delete" action in the popover, you can confirm the deletion but nothing actually happens (the source filter isn't deleted).

Show name of source filter in delete confirmation modal

It would be nice if the modal showed the name of the source filter you're deleting so you can be sure of exactly what your action is going to do. Based on feedback I've gotten from Gail, I'm 128% confident that she would say the title should say, "Delete source filter '${sourceFilterName}'?"

cjcenizal · 2018-02-12T20:16:23Z

...sections/indices/edit_index_pattern/source_filters_table/components/add_filter/add_filter.js

+
+export class AddFilter extends Component {
+  static propTypes = {
+    addFilter: PropTypes.func.isRequired,


Minor nit, but how about onAddFilter to emphasize that this is a callback?

cjcenizal · 2018-02-12T20:17:43Z

...agement/sections/indices/edit_index_pattern/source_filters_table/components/header/header.js

+} from '@elastic/eui';
+
+export const Header = () => (
+  <div>


We could use Fragment here instead. https://reactjs.org/docs/fragments.html

Actually, there seems to be issues with using enzyme and fragments: enzymejs/enzyme#1213

I'm lending towards leaving as is until those issues are cleaned up.

Thoughts?

Oh good call! Yes I agree, let's leave it.

cjcenizal · 2018-02-12T20:54:59Z

.../management/sections/indices/edit_index_pattern/source_filters_table/source_filters_table.js

-          this.$scope.indexPattern.sourceFilters = [...this.all(), { value }];
-          return this.save();
-        }
+  fetchFilters = () => {


Could you explain what's happening here? I'm used to seeing fetch used in conjunction with some async logic like an API request but it's not clear to me if that's happening here.

I changed the function name. Is that more clear? There is no async action happening here.

Honestly, I don't like the new function name. Recommendations welcome!

chrisronline · 2018-02-13T15:14:30Z

Disallow empty source filters
Can we disable the "Add" button until a non-empty string has been entered into the input field? I created some empty string source filters and they're basically useless.

Sure

Save on Enter
When I edited a source filter, it took me a few seconds to find the save button. How about we allow the user to save the source filter when they hit enter?

Yes!

Also, I'm not sure it makes sense to allow you to delete a source filter while you're editing it. Can we replace the multi-action popover with a single "Save" EuiButtonEmpty when you're in editing mode?

I could, but we wouldn't be able to add the Cancel button

Cancel editing mode
For that matter, how do you cancel editing mode? It would be nice if you could click a "Cancel" button and also hit ESC to exit editing mode.

Added!

Clicking save needs to close the popover
If you edit a source filter and then hit save, the popover remains open. Then if you click the "Delete" action in the popover, you can confirm the deletion but nothing actually happens (the source filter isn't deleted).

This feels like an issue with EuiTableOfRecords. It controls mouse events and hides and shows the action panel at it's own will.

Show name of source filter in delete confirmation modal
It would be nice if the modal showed the name of the source filter you're deleting so you can be sure of exactly what your action is going to do. Based on feedback I've gotten from Gail, I'm 128% confident that she would say the title should say, "Delete source filter '${sourceFilterName}'?"

Done (FYI, it didn't show it before so I was just keeping the status quo but I do agree)

@cjcenizal

elasticmachine · 2018-02-13T16:12:02Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

cjcenizal · 2018-02-13T18:34:10Z

.../management/sections/indices/edit_index_pattern/source_filters_table/source_filters_table.js

-          this.$scope.indexPattern.sourceFilters = [...this.all(), { value }];
-          return this.save();
-        }
+  putFiltersInState = () => {


updateFilters?

bmcconaghy

LGTM

bmcconaghy · 2018-02-16T14:05:26Z

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

+    return 'table';
+  }
+}));
+


I'm not a huge fan of all this mocking. Seems like it will take some maintenance to keep this matching the UI components you are using, and then you miss anything that breaks as a result of the EUI stuff changing.

jen-huang · 2018-02-16T19:30:51Z

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

+    }
+
+    return {
+      recordId: 'id',


I don't think this data has an id field. So this makes all the gear icons show up when any part of the table is hovered over, as opposed to just that row's gear icon:

Not sure what unique property can be used, since this accepts duplicate values...

jen-huang · 2018-02-16T19:35:00Z

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

+          dataType: 'string',
+          sortable: true,
+          render: value => {
+            if (this.state.editingFilter === value) {


Apparently it's possible to have two filters with the same value, so this opens both of them with edit is clicked:

Yup, good catch. I think I need to keep track of a client-side-only id and assign it to each filter

jen-huang · 2018-02-16T19:39:53Z

.../management/sections/indices/edit_index_pattern/source_filters_table/source_filters_table.js

+  }
+
+  deleteFilter = async () =>  {
+    const { indexPattern, onAddOrRemoveFilter } = this.props;


This deletes filters with the same value:

jen-huang · 2018-02-16T19:43:45Z

...n/source_filters_table/components/add_filter/__tests__/__snapshots__/add_filter.test.js.snap

+      fullWidth={true}
+      isLoading={false}
+      onChange={[Function]}
+      placeholder="source filter, accepts wildcards (e.g., \`user*\` to filter fields starting with \\\\'user\\\\')"


Perhaps one too many \s 🙂 It shows up in the placeholder:

chrisronline · 2018-02-21T21:16:09Z

Thanks for the great feedback @jen-huang @bmcconaghy @cjcenizal!

Ready for another pass!

elasticmachine · 2018-02-21T22:05:02Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

cjcenizal · 2018-02-21T23:12:03Z

.../management/sections/indices/edit_index_pattern/source_filters_table/source_filters_table.js

-            .finally(() => this.saving = false);
-        }
+  updateFilters = () => {
+    const filters = this.props.indexPattern.sourceFilters.map(filter => ({


I think we need to add some logic to handle the case when sourceFilters is undefined. I'm getting this error:

This seems to work:

const filters = this.props.indexPattern.sourceFilters ? this.props.indexPattern.sourceFilters.map(filter => ({ ...filter, clientId: ++this.clientSideId, })) : [];

cjcenizal · 2018-02-21T23:14:08Z

...sections/indices/edit_index_pattern/source_filters_table/components/add_filter/add_filter.js

+        </EuiFlexItem>
+        <EuiFlexItem>
+          <EuiButton
+            isDisabled={filter.length === 0}


How about filter.trim().length === 0 to account for when an empty string is entered?

cjcenizal · 2018-02-21T23:16:55Z

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

+  onEditingFilterChange = e => this.setState({ editingFilterValue: e.target.value })
+
+  onEditFieldKeyPressed = ({ charCode }) => {
+    if (keyCodes.ENTER === charCode) {


Can we handle the ESC key too, and call stopEditingFilter when it's pressed?

elasticmachine · 2018-02-22T15:35:12Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

cjcenizal

Here's the UI suggested by @snide:

After talking about it with @chrisronline we decided to keep the UI as close to the existing one as possible, using the solution I proposed. Then we can improve the editing experience using Dave's solution in a later iteration.

cjcenizal · 2018-02-22T00:55:42Z

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

+            return (<em>The source filter doesn&apos;t match any known fields.</em>);
+          }
+        },
+        {


Instead of using the built-in actions property here, how about we define our own actions?

{ name: '', align: RIGHT_ALIGNMENT, width: 100, render: (filter) => { if (this.state.editingFilterId) { return ( <Fragment> <EuiButtonIcon size="s" onClick={() => { saveFilter({ filterId: this.state.editingFilterId, newFilterValue: this.state.editingFilterValue, }); this.stopEditingFilter(); }} iconType="checkInCircleFilled" aria-label="Save" /> <EuiButtonIcon size="s" onClick={() => { this.stopEditingFilter(); }} iconType="cross" aria-label="Cancel" /> </Fragment> ); } return ( <Fragment> <EuiButtonIcon size="s" color="danger" onClick={() => deleteFilter(filter)} iconType="trash" aria-label="Delete" /> <EuiButtonIcon size="s" onClick={() => this.startEditingFilter(filter.clientId, filter.value)} iconType="pencil" aria-label="Edit" /> </Fragment> ); }, }

This will result in a UI like this:

Btw, RIGHT_ALIGNMENT is imported like this:

import { RIGHT_ALIGNMENT } from '@elastic/eui';

elasticmachine · 2018-03-02T20:50:24Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

chrisronline · 2018-03-06T00:45:36Z

I added a new feature here: 2cb0dc0

The UX feels better to update the matches as the user is editing the filter itself.

elasticmachine · 2018-03-06T01:30:51Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

chrisronline · 2018-03-09T15:33:07Z

jenkins, test this

elasticmachine · 2018-03-09T16:08:14Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

cjcenizal

🎾 Awesome! I noticed one last bit of UX polish we can do.

cjcenizal · 2018-03-09T21:07:27Z

.../management/sections/indices/edit_index_pattern/source_filters_table/source_filters_table.js

+        <Header />
+        <AddFilter onAddFilter={this.onAddFilter} />
+        <EuiSpacer size="l" />
+        {isSaving ? <EuiLoadingSpinner /> : null}


Can we remove the spinner?

cjcenizal · 2018-03-09T21:07:30Z

.../management/sections/indices/edit_index_pattern/source_filters_table/source_filters_table.js

+        <AddFilter onAddFilter={this.onAddFilter} />
+        <EuiSpacer size="l" />
+        {isSaving ? <EuiLoadingSpinner /> : null}
+        {filteredFilters.length > 0 ? (


And can we always show the table? It should display a "no items" message if it's empty, I think.

elasticmachine · 2018-03-09T22:14:30Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

* Source filters table * Updates * Handle no source filters * PR feedback * Fix merge issues * PR feedback * PR feedback * PR feedback * Upgrade to 3.3.0 which allows us to use Fragments in enzyme tests * Use EuiInMemoryTable instead * Remove dead code and simplify some tests * Dynamically update the matches as the user edits the filter * Apparently, this has been using the wrong parameter name * Restructure to stop storing computed data and add reselect helper * Reselect is tiny, just use that * PR feedback * Fix merge issues * PR feedback

chrisronline · 2018-03-12T13:43:02Z

Backport

6.x: 64739f8

chrisronline added review :Management v7.0.0 v6.3.0 labels Feb 9, 2018

chrisronline self-assigned this Feb 9, 2018

chrisronline requested review from jen-huang and bmcconaghy February 9, 2018 18:36

chrisronline mentioned this pull request Feb 12, 2018

[META] EUIfication #15721

Closed

47 tasks

cjcenizal suggested changes Feb 13, 2018

View reviewed changes

chrisronline force-pushed the eui/source_filters_table branch from 3f2bad2 to 84485a2 Compare February 13, 2018 15:20

cjcenizal reviewed Feb 13, 2018

View reviewed changes

bmcconaghy approved these changes Feb 16, 2018

View reviewed changes

jen-huang requested changes Feb 16, 2018

View reviewed changes

chrisronline force-pushed the eui/source_filters_table branch from 84485a2 to ba19e0d Compare February 21, 2018 21:16

cjcenizal reviewed Feb 21, 2018

View reviewed changes

cjcenizal suggested changes Feb 22, 2018

View reviewed changes

chrisronline force-pushed the eui/source_filters_table branch from 86b8333 to b4d0763 Compare March 2, 2018 20:04

chrisronline force-pushed the eui/source_filters_table branch from b4d0763 to 1196445 Compare March 5, 2018 23:59

cjcenizal approved these changes Mar 9, 2018

View reviewed changes

chrisronline added 18 commits March 9, 2018 16:22

Source filters table

6ef479f

Updates

524c243

Handle no source filters

96464ed

PR feedback

08fe70d

Fix merge issues

61099b5

PR feedback

d9712b0

PR feedback

b34015e

PR feedback

3c45ca3

Upgrade to 3.3.0 which allows us to use Fragments in enzyme tests

2c59c5b

Use EuiInMemoryTable instead

0367935

Remove dead code and simplify some tests

3e8b129

Dynamically update the matches as the user edits the filter

2ff1a57

Apparently, this has been using the wrong parameter name

ec7ea3e

Restructure to stop storing computed data and add reselect helper

e8de5ff

Reselect is tiny, just use that

3af30dd

PR feedback

0aef8f8

Fix merge issues

ffe173f

PR feedback

5f0d1be

chrisronline force-pushed the eui/source_filters_table branch from 0a3bb35 to 5f0d1be Compare March 9, 2018 21:29

chrisronline merged commit 0caa5a8 into elastic:master Mar 9, 2018

chrisronline deleted the eui/source_filters_table branch March 9, 2018 22:20

chrisronline mentioned this pull request Mar 9, 2018

[6.x] [Management] Source filters table in React (#16649) #17078

Merged

cjcenizal mentioned this pull request Mar 12, 2018

Componentization: List Index Pattern source filters using Table component #11905

Closed

[Management] Source filters table in React #16649

[Management] Source filters table in React #16649

Conversation

chrisronline commented Feb 9, 2018

elasticmachine commented Feb 9, 2018

💔 Build Failed

elasticmachine commented Feb 9, 2018

💚 Build Succeeded

cjcenizal left a comment • edited Loading

Choose a reason for hiding this comment

Disallow empty source filters

Save on Enter

Cancel editing mode

Clicking save needs to close the popover

Show name of source filter in delete confirmation modal

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

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

chrisronline commented Feb 13, 2018

elasticmachine commented Feb 13, 2018

💚 Build Succeeded

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

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

chrisronline commented Feb 21, 2018

elasticmachine commented Feb 21, 2018

💔 Build Failed

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cjcenizal Feb 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Feb 22, 2018

💚 Build Succeeded

cjcenizal left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Mar 2, 2018

💚 Build Succeeded

chrisronline commented Mar 6, 2018

elasticmachine commented Mar 6, 2018

💚 Build Succeeded

chrisronline commented Mar 9, 2018

elasticmachine commented Mar 9, 2018

💔 Build Failed

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

elasticmachine commented Mar 9, 2018

💚 Build Succeeded

chrisronline commented Mar 12, 2018

cjcenizal left a comment •

edited

Loading

cjcenizal Feb 21, 2018 •

edited

Loading

cjcenizal left a comment •

edited

Loading