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.

Sign up for GitHub

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

[Management] Scripted fields table in React #16604

Merged
merged 6 commits into from
Feb 12, 2018
Merged

[Management] Scripted fields table in React #16604

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0c6b0c0
First pass at new scripted fields table
chrisronline Feb 7, 2018
1aa89e6
Ensure we destroy when we switch tabs
chrisronline Feb 8, 2018
d2cae1f
PR feedback
chrisronline Feb 8, 2018
fe6e112
Fix functional tests
chrisronline Feb 8, 2018
70cdca1
Fix functional tests
chrisronline Feb 8, 2018
1376df5
PR feedback and more tests
chrisronline Feb 9, 2018
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
5 changes: 1 addition & 4 deletions ...gins/kibana/public/management/sections/indices/edit_index_pattern/edit_index_pattern.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,10 +145,7 @@
class="fields indexed-fields"
></indexed-fields-table>

<scripted-fields-table
ng-show="state.tab == 'scriptedFields'"
class="fields scripted-fields"
></scripted-fields-table>
<div id="reactScriptedFieldsTable"></div>

<source-filters-table
ng-show="state.tab == 'sourceFilters'"
Expand Down
45 changes: 44 additions & 1 deletion ...lugins/kibana/public/management/sections/indices/edit_index_pattern/edit_index_pattern.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
import _ from 'lodash';
import './index_header';
import './indexed_fields_table';
import './scripted_fields_table';
import './scripted_field_editor';
import './source_filters_table';
import { KbnUrlProvider } from 'ui/url';
Expand All @@ -11,6 +10,8 @@ import uiRoutes from 'ui/routes';
import { uiModules } from 'ui/modules';
import template from './edit_index_pattern.html';

import { renderScriptedFieldsTable, destroyScriptedFieldsTable } from './scripted_fields_table';

uiRoutes
.when('/management/kibana/indices/:indexPatternId', {
template,
Expand Down Expand Up @@ -79,6 +80,7 @@ uiModules.get('apps/management')

$scope.changeTab = function (obj) {
$state.tab = obj.index;
$scope.tryToRenderScriptedFieldsTable();
$state.save();
};

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

$scope.tryToRenderScriptedFieldsTable = function () {
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, I'll change it

if ($state.tab === 'scriptedFields') {
Copy link
Contributor

Choose a reason for hiding this comment

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

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 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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)

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

$scope.$$postDigest($scope.renderScriptedFieldsTable);
} else {
destroyScriptedFieldsTable();
}
};

$scope.renderScriptedFieldsTable = function () {
Copy link
Contributor

@cjcenizal cjcenizal Feb 8, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good!

renderScriptedFieldsTable(
$scope.indexPattern,
$scope.fieldFilter,
$scope.scriptedFieldLanguageFilter,
{
redirectToRoute: (obj, route) => {
$scope.kbnUrl.redirectToRoute(obj, route);
$scope.$apply();
},
getRouteHref: (obj, route) => $scope.kbnUrl.getRouteHref(obj, route),
}
);
};

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

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

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

$scope.tryToRenderScriptedFieldsTable();
});
290 changes: 290 additions & 0 deletions ...x_pattern/scripted_fields_table/__tests__/__snapshots__/scripted_field_table.test.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,290 @@
// Jest Snapshot v1, https://goo.gl/fbAQLP

exports[`ScriptedFieldsTable should filter based on the lang filter 1`] = `
<ScriptedFieldsTable
helpers={
Object {
"getRouteHref": [Function],
"redirectToRoute": [Function],
}
}
indexPattern={
Object {
"getScriptedFields": [Function],
}
}
scriptedFieldLanguageFilter="painless"
>
<div>
<header />
<call-outs
deprecatedLangsInUse={
Array [
"somethingElse",
]
}
painlessDocLink="painlessDocs"
/>
<eui-button
data-test-subj="addScriptedFieldLink"
href={undefined}
>
Add Scripted Field
</eui-button>
<eui-spacer
size="l"
/>
<Table
deleteField={[Function]}
editField={[Function]}
indexPattern={
Object {
"getScriptedFields": [Function],
}
}
model={
Object {
"criteria": Object {
"page": Object {
"index": 0,
"size": 10,
},
"sort": Object {
"direction": "asc",
"field": "name",
},
},
"data": Object {
"records": Array [
Object {
"lang": "painless",
"name": "ScriptedField",
"script": "x++",
},
Object {
"lang": "painless",
"name": "JustATest",
"script": "z++",
},
],
"totalRecordCount": 2,
},
}
}
onDataCriteriaChange={[Function]}
>
table
</Table>
</div>
</ScriptedFieldsTable>
`;

exports[`ScriptedFieldsTable should filter based on the query bar 1`] = `
<ScriptedFieldsTable
fieldFilter="Just"
helpers={
Object {
"getRouteHref": [Function],
"redirectToRoute": [Function],
}
}
indexPattern={
Object {
"getScriptedFields": [Function],
}
}
>
<div>
<header />
<call-outs
deprecatedLangsInUse={Array []}
painlessDocLink="painlessDocs"
/>
<eui-button
data-test-subj="addScriptedFieldLink"
href={undefined}
>
Add Scripted Field
</eui-button>
<eui-spacer
size="l"
/>
<Table
deleteField={[Function]}
editField={[Function]}
indexPattern={
Object {
"getScriptedFields": [Function],
}
}
model={
Object {
"criteria": Object {
"page": Object {
"index": 0,
"size": 10,
},
"sort": Object {
"direction": "asc",
"field": "name",
},
},
"data": Object {
"records": Array [
Object {
"lang": "painless",
"name": "JustATest",
"script": "z++",
},
],
"totalRecordCount": 1,
},
}
}
onDataCriteriaChange={[Function]}
>
table
</Table>
</div>
</ScriptedFieldsTable>
`;

exports[`ScriptedFieldsTable should render normally 1`] = `
<ScriptedFieldsTable
helpers={
Object {
"getRouteHref": [Function],
"redirectToRoute": [Function],
}
}
indexPattern={
Object {
"getScriptedFields": [Function],
}
}
>
<div>
<header />
<call-outs
deprecatedLangsInUse={Array []}
painlessDocLink="painlessDocs"
/>
<eui-button
data-test-subj="addScriptedFieldLink"
href={undefined}
>
Add Scripted Field
</eui-button>
<eui-spacer
size="l"
/>
<Table
deleteField={[Function]}
editField={[Function]}
indexPattern={
Object {
"getScriptedFields": [Function],
}
}
model={
Object {
"criteria": Object {
"page": Object {
"index": 0,
"size": 10,
},
"sort": Object {
"direction": "asc",
"field": "name",
},
},
"data": Object {
"records": Array [
Object {
"lang": "painless",
"name": "ScriptedField",
"script": "x++",
},
Object {
"lang": "painless",
"name": "JustATest",
"script": "z++",
},
],
"totalRecordCount": 2,
},
}
}
onDataCriteriaChange={[Function]}
>
table
</Table>
</div>
</ScriptedFieldsTable>
`;

exports[`ScriptedFieldsTable should show a message if there are no scripted fields 1`] = `
<ScriptedFieldsTable
helpers={
Object {
"getRouteHref": [Function],
"redirectToRoute": [Function],
}
}
indexPattern={
Object {
"getScriptedFields": [Function],
}
}
>
<div>
<header />
<call-outs
deprecatedLangsInUse={Array []}
painlessDocLink="painlessDocs"
/>
<eui-button
data-test-subj="addScriptedFieldLink"
href={undefined}
>
Add Scripted Field
</eui-button>
<eui-spacer
size="l"
/>
<Table
deleteField={[Function]}
editField={[Function]}
indexPattern={
Object {
"getScriptedFields": [Function],
}
}
model={
Object {
"criteria": Object {
"page": Object {
"index": 0,
"size": 10,
},
"sort": Object {
"direction": "asc",
"field": "name",
},
},
"data": Object {
"records": Array [],
"totalRecordCount": 0,
},
}
}
onDataCriteriaChange={[Function]}
>
table
</Table>
<eui-text>
No scripted fields found.
</eui-text>
</div>
</ScriptedFieldsTable>
`;
Loading