Factor out column manipulation in the doc table #10681

weltenwort · 2017-03-03T12:06:00Z

This refactoring effort turns the implicit manipulation of the set of
columns that was scattered throughout the doc table and the field
chooser into explicit function calls. This brings with it the following
improvements:

The column manipulation code is not duplicated (DRY)
The controller stays in control (IOC)
If the required functions are not provided by the controller,
manipulation of the columns is disabled.

Additionally, the discover_field now uses a properly isolated scope
instead of accessing inherited properties of the scope.

As a side benefit, this enables us to restrict the ability of the user to change the columns in saved search panels on dashboards. See #9523 for a (currently unresolved) discussion on that topic.

stacey-gammon

This looks awesome Felix! Much improved.

stacey-gammon · 2017-03-29T14:52:46Z

src/core_plugins/kbn_doc_views/public/views/table.js

      },
      controller: function ($scope) {
        $scope.mapping = $scope.indexPattern.fields.byName;
        $scope.flattened = $scope.indexPattern.flattenHit($scope.hit);
        $scope.formatted = $scope.indexPattern.formatHit($scope.hit);
        $scope.fields = _.keys($scope.flattened).sort();

-        $scope.toggleColumn = function (fieldName) {
-          _.toggleInOut($scope.columns, fieldName);
+        $scope.canToggleColumn = function canToggleColumn() {


This is called per row so could maybe pull out the calculation so it doesn't run every time?

const canToggleColumns = _.isFunction($scope.onAddColumn) && _.isFunction($scope.onRemoveColumn); $scope.canToggleColumns = () => canToggleColumns;

Though it's probably like a nanosecond extra for those _.isFunction calculations and negligible in the long run, so feel free to ignore. :)

Though I do think I'd call it canToggleColumns which makes it clearer it's either on or off for all.

Good point about the plural, thanks!

To get the precomputed value correct I would have to set up a watch (because $scope.onAddColumn and $scope.onRemoveColumn could change over time), which would probably already negate some of the benefit. I'll take a look at the profiler to see if it's worth it 😉

stacey-gammon · 2017-03-29T15:00:05Z

src/core_plugins/kibana/public/dashboard/panel/panel.js

+          };
+
+          $scope.removeColumn = function removeColumn(columnName) {
+            $scope.savedObj.searchSource.get('index').popularizeField(columnName, 1);


Interesting. Is this new? A quick test seems to indicate that previously adjusting the columns via dashboard didn't affect popularity score. Makes sense to add it here, though.

You're right, this was previously not consistently triggered for the different ways to add columns. I actually didn't notice, but I guess we could keep it.

I would think it would make sense to add it here as well.

lukasolson

I left a few tiny nitpicks below, but really this LGTM. Thanks for taking this on!

lukasolson · 2017-03-30T23:35:53Z

src/core_plugins/kibana/public/discover/components/field_chooser/discover_field.js

    link: function ($scope, $elem) {
      let detailsElem;
-      let detailScope = $scope.$new();
+      let detailScope = null;


Any reason to initialize this to null rather than just leaving uninitialized?

no, just habit from languages where this could blow up 😉 thanks for pointing it out

lukasolson · 2017-03-30T23:37:23Z

src/core_plugins/kibana/public/discover/components/field_chooser/field_chooser.js

@@ -155,7 +152,7 @@ app.directive('discFieldChooser', function ($location, globalState, config, $rou
        $scope.indexPattern.popularizeField(fieldName, 1);
      };

-      $scope.vizLocation = function (field) {
+      function getVizualizeUrl(field) {


Nit: Misspelling of "visualize"

zZzZz, thanks - that's the downside of auto-completion

lukasolson · 2017-03-30T23:41:58Z

src/ui/public/doc_table/components/table_header.html

+        ng-if="canMoveColumnRight(name)"
+        tooltip-append-to-body="1"
+        tooltip="Move column to the right"
+      ></i>


Soooo much more readable. Thanks 👍

weltenwort · 2017-03-31T15:42:43Z

jenkins, test this (and stop failing arbitrarily)

This refactoring effort turns the implicit manipulation of the set of columns that was scattered throughout the doc table and the field chooser into explicit function calls. This brings with it the following improvements: * The column manipulation code is not duplicated (DRY) * The controller stays in control (IOC) * If the required functions are not provided by the controller, manipulation of the columns is disabled. Additionally, the `discover_field` now uses a properly isolated scope instead of accessing inherited properties of the scope.

* Factor out column manipulation in the doc table This refactoring effort turns the implicit manipulation of the set of columns that was scattered throughout the doc table and the field chooser into explicit function calls. This brings with it the following improvements: * The column manipulation code is not duplicated (DRY) * The controller stays in control (IOC) * If the required functions are not provided by the controller, manipulation of the columns is disabled. Additionally, the `discover_field` now uses a properly isolated scope instead of accessing inherited properties of the scope. * Make filter addition and removal in tests more reliable * Change function name to plural, move up ng-if * Remove inconsistent variable initialization * Fix function name typo * Save the state in the action instead of a $watch

Backports PR #10681 * Factor out column manipulation in the doc table This refactoring effort turns the implicit manipulation of the set of columns that was scattered throughout the doc table and the field chooser into explicit function calls. This brings with it the following improvements: * The column manipulation code is not duplicated (DRY) * The controller stays in control (IOC) * If the required functions are not provided by the controller, manipulation of the columns is disabled. Additionally, the `discover_field` now uses a properly isolated scope instead of accessing inherited properties of the scope. * Make filter addition and removal in tests more reliable * Change function name to plural, move up ng-if * Remove inconsistent variable initialization * Fix function name typo * Save the state in the action instead of a $watch

weltenwort added Feature:Discover Discover Application :Discovery review v5.4.0 v6.0.0 labels Mar 3, 2017

weltenwort self-assigned this Mar 3, 2017

weltenwort mentioned this pull request Mar 3, 2017

Discover Document Table Cleanup #10682

Closed

3 tasks

weltenwort force-pushed the refactoring/discover-table-dry-column-change branch from d577e31 to aa68edb Compare March 3, 2017 15:51

weltenwort force-pushed the refactoring/discover-table-dry-column-change branch 4 times, most recently from 7eeabcd to d9131a3 Compare March 28, 2017 15:30

weltenwort requested review from lukasolson and Bargs March 28, 2017 18:58

This was referenced Mar 28, 2017

Changes to Saved Search objects do not reflect in Dashboard without re-adding #9523

Closed

Restrict column configations in saved searches on a dashboard #10920

Closed

weltenwort requested a review from stacey-gammon March 29, 2017 13:03

stacey-gammon approved these changes Mar 29, 2017

View reviewed changes

lukasolson approved these changes Mar 30, 2017

View reviewed changes

weltenwort force-pushed the refactoring/discover-table-dry-column-change branch from d9131a3 to 68a50ee Compare March 31, 2017 11:04

weltenwort force-pushed the refactoring/discover-table-dry-column-change branch from f57d78d to 1850e2c Compare April 3, 2017 08:53

weltenwort added 6 commits April 3, 2017 17:22

Make filter addition and removal in tests more reliable

1ac77bc

Change function name to plural, move up ng-if

085365d

Remove inconsistent variable initialization

a936d5f

Fix function name typo

4c38e4a

Save the state in the action instead of a $watch

7f2bda8

weltenwort force-pushed the refactoring/discover-table-dry-column-change branch from 1850e2c to 7f2bda8 Compare April 3, 2017 15:23

weltenwort merged commit 18e0252 into elastic:master Apr 3, 2017

weltenwort mentioned this pull request Apr 3, 2017

[5.x] Factor out column manipulation in the doc table #11006

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Factor out column manipulation in the doc table #10681

Factor out column manipulation in the doc table #10681

weltenwort commented Mar 3, 2017 •

edited

Loading

stacey-gammon left a comment

stacey-gammon Mar 29, 2017

weltenwort Mar 29, 2017

stacey-gammon Mar 29, 2017

weltenwort Mar 29, 2017

lukasolson Mar 30, 2017

lukasolson left a comment

lukasolson Mar 30, 2017

weltenwort Mar 31, 2017

lukasolson Mar 30, 2017

weltenwort Mar 31, 2017

lukasolson Mar 30, 2017

weltenwort commented Mar 31, 2017

Factor out column manipulation in the doc table #10681

Factor out column manipulation in the doc table #10681

Conversation

weltenwort commented Mar 3, 2017 • edited Loading

stacey-gammon 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

lukasolson 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

weltenwort commented Mar 31, 2017

weltenwort commented Mar 3, 2017 •

edited

Loading