[Management] Scripted fields table in React #16604

chrisronline · 2018-02-08T14:24:02Z

Fixes #16539
Relates to #15721

This PR converts the scripted field tables to using React/EUI.

bmcconaghy · 2018-02-08T15:02:19Z

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

@@ -1,7 +1,7 @@
 import _ from 'lodash';
 import './index_header';
 import './indexed_fields_table';
-import './scripted_fields_table';
+// import './scripted_fields_table';


commented out code begone

bmcconaghy · 2018-02-08T15:10:35Z

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

@@ -140,4 +143,43 @@ uiModules.get('apps/management')
      $scope.indexPattern.timeFieldName = field.name;
      return $scope.indexPattern.save();
    };
+
+    $scope.tryToRenderScriptedFieldsTable = function () {
+      if ($state.tab === 'scriptedFields') {


This file and a couple of others are just so bound up in Angular. Since we are supposed to be running away from Angular anyway, I wonder if it makes sense to take this Reactification work farther ?

Yea I agree, but I think the focus should be on the tables for now to ensure those can go out sooner rather than later. Once the tables are in, I want to take a pass on the shell part (edit_index_pattern)

I guess I just worry about people looking at these squashed merged changes and taking them as patterns.

And maybe there are bugs in all this glue code. Just seems better to get out of this to me.

Yea I don't want that to happen, but converting this to React is probably more work than it seems because this is a shell that renders angular directives and getting those to work within a React shell seems complicated too.

What if I renamed the variables/functions in here to names that directly state to not use this and it's just a stopgap?

elasticmachine · 2018-02-08T15:11:27Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-02-08T16:18:05Z

💔 Build Failed

continuous-integration/kibana-ci/pull-request

elasticmachine · 2018-02-08T19:09:48Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

cjcenizal

Rad! This looks sweet. I had a few minor suggestions. Can we delete scripted_fields_table_angular.js? Looks like it's not in use.

cjcenizal · 2018-02-08T19:37:50Z

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

@@ -140,4 +142,45 @@ uiModules.get('apps/management')
      $scope.indexPattern.timeFieldName = field.name;
      return $scope.indexPattern.save();
    };
+
+    $scope.tryToRenderScriptedFieldsTable = function () {


Does this need to be a scope method? Can we just make it a local function?

And a minor nit: can we rename it to updateScriptedFieldsTab? I wasn't sure how to intuit its role from the word "try" in the name.

Sure, I'll change it

cjcenizal · 2018-02-08T19:43:58Z

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

+      }
+    };
+
+    $scope.renderScriptedFieldsTable = function () {


I think it makes sense to redefine this as a local function instead of a scope method (unless it needs to be on scope, but I don't think it does), and to move the logic from scripted_fields_table/index.js into here instead.

This makes sense to me since it mirrors how this will be structured once we migrate to React. The owner of the <ScriptedFieldsTable /> instance will be responsible for providing the props it needs. I think it also simplifies the code a bit.

So it would be:

function renderScriptedFieldsTable() { const node = document.getElementById(DOM_ELEMENT_ID); if (!node) { return; } render( <ScriptedFieldsTable indexPattern={$scope.indexPattern} fieldFilter={$scope.fieldFilter} scriptedFieldLanguageFilter={$scope.scriptedFieldLanguageFilter} helpers={{ redirectToRoute: (obj, route) => { $scope.kbnUrl.redirectToRoute(obj, route); $scope.$apply(); }, getRouteHref: (obj, route) => $scope.kbnUrl.getRouteHref(obj, route), }} />, node, ); }

Sounds good!

cjcenizal · 2018-02-08T19:50:47Z

.../sections/indices/edit_index_pattern/scripted_fields_table/components/call_outs/call_outs.js

+  return (
+    <div>
+      <EuiCallOut
+        title="Deprecation Warning"


According to our writing guidelines, this should be sentence case, "Deprecation warning." @gchaps Could you look over the copy below and provide feedback?

cjcenizal · 2018-02-08T19:53:34Z

...anagement/sections/indices/edit_index_pattern/scripted_fields_table/scripted_fields_table.js

+    );
+  }
+
+  renderNoFieldsFound() {


The empty table looks a little weird:

At a later point the Design team will come up with a nice solution for empty table states which we can use here. For now can we just not render anything, including the table, when there are no fields?

My comments for the screen above:

Tab names should have init caps:

Fields
Scripted fields
Source filters

I feel that this paragraph is overwritten

These scripted fields are computed on the fly from your data. They can be used in visualizations and displayed in your documents. However, they cannot be searched. You can manage them here and add new ones as you see fit, but be careful, scripts can be tricky!

My recommendation:

You can use scripted fields in visualizations and display them in your documents. However, you cannot search scripted fields.

Button names use sentence case:

Add Scripted Field -> Add scripted field

No scripted fields found. -> No scripted fields found (no ending punctuation)

For the screen below:

1)The delete button in the upper right is in red and the Delete button in the confirmation modal is blue. The color should be consistent. Red is typically used for Delete.

Rewrite for the sentence starting with "This page.." as follows:

This page lists every field in the log* index and the field's associated core type as recorded by Elasticsearch. To change a field type, use the Elasticsearch Mapping API.

For the Add button on the on the Source filter page, our guidelines say to follow Add by a noun, so

Add source filter

I recommend cutting down on the text under Source filter and making it easier to scan, The text feels like a help topic

Thanks!

1)The delete button in the upper right is in red and the Delete button in the confirmation modal is blue. The color should be consistent. Red is typically used for Delete.

@cjcenizal Is this supported in EUI now? I don't see a prop that looks like it will do it. Want me to add something?

@chrisronline Yes please! Thanks!

jen-huang

This pattern is super similar to what I've done so far for the indexed fields table so that's awesome 😄I'll adjust what I have to match more closely with this (just small changes).

I second CJ's comments about removing scripted_fields_table_angular.js. I removed the old angular version for indexed fields table. Also +1 on hiding table when there aren't any fields to display.

Also saw a small bug where if you delete a field, the count on the tab doesn't update:

gchaps · 2018-02-09T00:05:08Z

.../sections/indices/edit_index_pattern/scripted_fields_table/components/call_outs/call_outs.js

+          We&apos;ve detected that the following deprecated languages are in use: {deprecatedLangsInUse.join(', ')}.
+          Support for these languages will be removed in the next major version of Kibana and Elasticsearch.
+          We recommend converting your scripted fields to <EuiLink href={painlessDocLink}>Painless</EuiLink>.
+        </p>


Here's my suggested rewrite. I'd prefer to see a more descriptive title than Deprecation Warning

Deprecated languages in use

The following deprecated languages are in use: {deprecatedLangsInUse.join(', ')}. Support for these languages will be removed in the next major version of Kibana and Elasticsearch. Convert you scripted fields to Painless to avoid any problems.

chrisronline · 2018-02-09T15:27:57Z

@jen-huang Great catch!

chrisronline · 2018-02-09T15:55:06Z

Thanks for the feedback everyone! PR is updated and ready for another pass!

elasticmachine · 2018-02-09T16:41:00Z

💚 Build Succeeded

continuous-integration/kibana-ci/pull-request

jen-huang · 2018-02-09T21:43:54Z

@chrisronline Another minor thing - long field names overlap the next column a bit too soon:

I dealt with the same issue on indexed fields table. My fix includes setting width: '50%' on the field name column, '50px' on the actions column, and '120px' on the boolean value columns. YMMV since the columns here have different data.

chrisronline · 2018-02-09T21:58:51Z

Thanks @jen-huang! @cjcenizal does it make sense to fix something like that in EUI directly?

cjcenizal · 2018-02-09T22:13:35Z

@chrisronline Yes it absolutely does. We just need to remove the max-width: 20px style from the .euiTableCell class. And make sure it doesn't break anything in the table examples on the docs site.

chrisronline · 2018-02-12T14:36:00Z

Great, I made a ticket to address that

jen-huang

LGTM!

cjcenizal

💎 LGTM! I noticed that when a scripted field uses the default formatting the "Default" column is blank. Maybe it's worth showing the default value there?

chrisronline · 2018-02-12T19:56:07Z

Backport

6.x: 0060fd1

First pass at new scripted fields table

0c6b0c0

chrisronline self-assigned this Feb 8, 2018

chrisronline requested review from jen-huang and bmcconaghy February 8, 2018 14:26

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

Ensure we destroy when we switch tabs

1aa89e6

bmcconaghy reviewed Feb 8, 2018

View reviewed changes

chrisronline added 2 commits February 8, 2018 10:22

PR feedback

d2cae1f

Fix functional tests

fe6e112

Fix functional tests

70cdca1

bmcconaghy approved these changes Feb 8, 2018

View reviewed changes

cjcenizal suggested changes Feb 8, 2018

View reviewed changes

jen-huang requested changes Feb 8, 2018

View reviewed changes

gchaps reviewed Feb 9, 2018

View reviewed changes

PR feedback and more tests

1376df5

chrisronline mentioned this pull request Feb 12, 2018

Fix long name overlap in Table elastic/eui#389

Closed

jen-huang approved these changes Feb 12, 2018

View reviewed changes

cjcenizal approved these changes Feb 12, 2018

View reviewed changes

chrisronline merged commit b932467 into elastic:master Feb 12, 2018

chrisronline deleted the eui/scripted_fields_table branch February 12, 2018 18:48

chrisronline mentioned this pull request Feb 12, 2018

[6.x] [Management] Scripted fields table in React (#16604) #16682

Merged

chrisronline mentioned this pull request Feb 12, 2018

[META] EUIfication #15721

Closed

47 tasks

cjcenizal mentioned this pull request Mar 12, 2018

Componentization: List Index Pattern scripted fields using Table component #11904

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Management] Scripted fields table in React #16604

[Management] Scripted fields table in React #16604

chrisronline commented Feb 8, 2018 •

edited

Loading

bmcconaghy Feb 8, 2018

bmcconaghy Feb 8, 2018

chrisronline Feb 8, 2018

bmcconaghy Feb 8, 2018

bmcconaghy Feb 8, 2018

chrisronline Feb 8, 2018

elasticmachine commented Feb 8, 2018

elasticmachine commented Feb 8, 2018

elasticmachine commented Feb 8, 2018

cjcenizal left a comment

cjcenizal Feb 8, 2018

cjcenizal Feb 8, 2018

chrisronline Feb 9, 2018

cjcenizal Feb 8, 2018 •

edited

Loading

chrisronline Feb 9, 2018

cjcenizal Feb 8, 2018

chrisronline Feb 9, 2018

cjcenizal Feb 8, 2018

gchaps Feb 8, 2018 •

edited

Loading

chrisronline Feb 9, 2018

cjcenizal Feb 9, 2018

jen-huang left a comment

gchaps Feb 9, 2018

chrisronline commented Feb 9, 2018

chrisronline commented Feb 9, 2018

elasticmachine commented Feb 9, 2018

jen-huang commented Feb 9, 2018 •

edited

Loading

chrisronline commented Feb 9, 2018

cjcenizal commented Feb 9, 2018

chrisronline commented Feb 12, 2018

jen-huang left a comment

cjcenizal left a comment

chrisronline commented Feb 12, 2018

[Management] Scripted fields table in React #16604

[Management] Scripted fields table in React #16604

Conversation

chrisronline commented Feb 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

elasticmachine commented Feb 8, 2018

💔 Build Failed

elasticmachine commented Feb 8, 2018

💔 Build Failed

elasticmachine commented Feb 8, 2018

💚 Build Succeeded

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

cjcenizal Feb 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gchaps Feb 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 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chrisronline commented Feb 9, 2018

chrisronline commented Feb 9, 2018

elasticmachine commented Feb 9, 2018

💚 Build Succeeded

jen-huang commented Feb 9, 2018 • edited Loading

chrisronline commented Feb 9, 2018

cjcenizal commented Feb 9, 2018

chrisronline commented Feb 12, 2018

jen-huang left a comment

Choose a reason for hiding this comment

cjcenizal left a comment

Choose a reason for hiding this comment

chrisronline commented Feb 12, 2018

chrisronline commented Feb 8, 2018 •

edited

Loading

cjcenizal Feb 8, 2018 •

edited

Loading

gchaps Feb 8, 2018 •

edited

Loading

jen-huang commented Feb 9, 2018 •

edited

Loading