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

Add registry for doc table details #6105

Merged
merged 7 commits into from
Feb 5, 2016
Merged
Show file tree
Hide file tree
Changes from 2 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
3 changes: 2 additions & 1 deletion src/plugins/kibana/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,8 @@ module.exports = function (kibana) {
'spyModes',
'fieldFormats',
'navbarExtensions',
'settingsSections'
'settingsSections',
'docTableDetailViews'
Copy link
Contributor

Choose a reason for hiding this comment

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

I like this as an extension point for plugins, but I think the existing doc viewers should use it (rather than being included directly). Perhaps they should get their own plugin, similar to the kbn_vislib_vis_types plugin.

Since there isn't any direct dependence on the docTable It may make sense to go generic with this export type and just call it docViews, then the docViewer (or any other component that wants to display docs) can utilize them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I created kbn_doc_views plugin and moved them there. The registry is now named docViews.

Copy link
Contributor

Choose a reason for hiding this comment

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

💃

],

injectVars: function (server, options) {
Expand Down
76 changes: 0 additions & 76 deletions src/ui/public/doc_viewer/doc_viewer.html

This file was deleted.

63 changes: 34 additions & 29 deletions src/ui/public/doc_viewer/doc_viewer.js
Original file line number Diff line number Diff line change
@@ -1,47 +1,52 @@
import _ from 'lodash';
import angular from 'angular';
import 'ace';
import html from 'ui/doc_viewer/doc_viewer.html';
import $ from 'jquery';
import 'ui/doc_viewer/doc_viewer.less';
define(function (require) {

import 'ui/doc_viewer/viewers/table.js';
import 'ui/doc_viewer/viewers/json.js';

define(function (require) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since this file is basically rewritten anyway, mind removing the AMD wrapper?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done


require('ui/modules').get('kibana')
.directive('docViewer', function (config, Private) {
const docTableDetailViews = Private(require('ui/registry/doc_table_detail_views'));
Copy link
Contributor

Choose a reason for hiding this comment

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

I suggest following the pattern used in https://github.com/elastic/kibana/pull/6102/files. We will be moving to it very soon

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

return {
restrict: 'E',
template: html,
scope: {
hit: '=',
indexPattern: '=',
filter: '=?',
columns: '=?'
},
link: {
pre($scope) {
$scope.aceLoaded = (editor) => {
editor.$blockScrolling = Infinity;
};
},

post($scope, $el, attr) {
// If a field isn't in the mapping, use this
$scope.mode = 'table';
$scope.mapping = $scope.indexPattern.fields.byName;
$scope.flattened = $scope.indexPattern.flattenHit($scope.hit);
$scope.hitJson = angular.toJson($scope.hit, true);
$scope.formatted = $scope.indexPattern.formatHit($scope.hit);
$scope.fields = _.keys($scope.flattened).sort();

$scope.toggleColumn = function (fieldName) {
_.toggleInOut($scope.columns, fieldName);
};
template: function ($el, $attr) {
const $viewer = $('<div class="doc-viewer">');
$el.append($viewer);
const $tabs = $('<ul class="nav nav-tabs">');
const $content = $('<div class="doc-viewer-content">');
$viewer.append($tabs);
$viewer.append($content);
docTableDetailViews.inOrder.forEach(view => {
const $tab = $(`<li ng-show="docViews['${view.title}'].shouldShow(hit)" ng-class="{active: mode == '${view.title}'}">
<a ng-click="mode='${view.title}'">${view.title}</a>
</li>`);
$tabs.append($tab);
const $ext = $(`<render-directive ng-show="mode == '${view.title}'" definition="docViews['${view.title}']" scope="innerScope">
</render-directive>`);
$ext.html(view.template);
$content.append($ext);
});
return $el.html();
},
controller: function ($scope) {
$scope.mode = docTableDetailViews.inOrder[0].title;
$scope.docViews = docTableDetailViews.byTitle;

$scope.showArrayInObjectsWarning = function (row, field) {
var value = $scope.flattened[field];
return _.isArray(value) && typeof value[0] === 'object';
};
}
$scope.innerScope = {
hit: $scope.hit,
indexPattern: $scope.indexPattern,
filter: $scope.filter,
columns: $scope.columns
};
}
};
});
Expand Down
16 changes: 16 additions & 0 deletions src/ui/public/doc_viewer/viewers/json.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,16 @@
<div
id="json-ace"
ng-model="hitJson"
readonly
ui-ace="{
useWrapMode: true,
onLoad: aceLoaded,
advanced: {
highlightActiveLine: false
},
rendererOptions: {
showPrintMargin: false,
maxLines: 4294967296
},
mode: 'json'
}"></div>
27 changes: 27 additions & 0 deletions src/ui/public/doc_viewer/viewers/json.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
import _ from 'lodash';
import angular from 'angular';
import 'ace';
import docTableDetailViewsRegistry from 'ui/registry/doc_table_detail_views';

import jsonHtml from './json.html';

docTableDetailViewsRegistry.register(function () {
return {
title: 'JSON',
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer if these objects didn't mix together the properties needed for the <render-directive> definition and extra properties needed for the docViewer. Maybe the object should look more like this?

return {
  title: ...
  order: ...
  directive: {
    template: ...,
    controller() { }
  }
};

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

order: 20,
template: jsonHtml,
shouldShow: () => true,
Copy link
Contributor

Choose a reason for hiding this comment

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

Ideally any doc view would be able to render any hit, so if you don't have a solid use case for shouldShow() I would prefer we drop it.

Otherwise you probably want to make _.constant(true) the default.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have a few doc views that I only want to show for certain hits. It looks at fields in the hit to determine if the doc view is useful or not (will it show something or will it just be a blank view). That's why I added the shouldShow.

Copy link
Contributor

Choose a reason for hiding this comment

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

Alright, works for me. I do think the shouldShow function should have a default though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed

controller: function ($scope) {
$scope.indexPattern = $scope.scope.indexPattern;
$scope.hit = $scope.scope.hit;
$scope.columns = $scope.scope.columns;
$scope.filter = $scope.scope.filter;

$scope.hitJson = angular.toJson($scope.hit, true);

$scope.aceLoaded = (editor) => {
editor.$blockScrolling = Infinity;
};
}
};
});
49 changes: 49 additions & 0 deletions src/ui/public/doc_viewer/viewers/table.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,49 @@
<table class="table table-condensed">
<tbody>
<tr ng-repeat="field in fields">
<td field-name="field"
field-type="mapping[field].type"
width="1%"
class="doc-viewer-field">
</td>
<td width="1%" class="doc-viewer-buttons" ng-if="filter">
<span ng-if="mapping[field].filterable">
<i ng-click="filter(mapping[field], flattened[field], '+')"
tooltip="Filter for value"
tooltip-append-to-body="1"
class="fa fa-search-plus"></i>
<i ng-click="filter(mapping[field], flattened[field],'-')"
tooltip="Filter out value"
tooltip-append-to-body="1"
class="fa fa-search-minus"></i>
</span>
<span ng-if="!mapping[field].filterable" tooltip="Unindexed fields can not be searched">
<i class="fa fa-search-plus text-muted"></i>
<i class="fa fa-search-minus text-muted"></i>
</span>
<span ng-if="columns">
<i ng-click="toggleColumn(field)"
tooltip="Toggle column in table"
tooltip-append-to-body="1"
class="fa fa-columns"></i>
</span>
</td>

<td>
<i ng-if="!mapping[field] && field[0] === '_'"
tooltip-placement="top"
tooltip="Field names beginning with _ are not supported"
class="fa fa-warning text-color-warning ng-scope doc-viewer-underscore"></i>
<i ng-if="!mapping[field] && field[0] !== '_' && !showArrayInObjectsWarning(doc, field)"
tooltip-placement="top"
tooltip="No cached mapping for this field. Refresh field list from the Settings > Indices page"
class="fa fa-warning text-color-warning ng-scope doc-viewer-no-mapping"></i>
<i ng-if="showArrayInObjectsWarning(doc, field)"
tooltip-placement="top"
tooltip="Objects in arrays are not well supported."
class="fa fa-warning text-color-warning ng-scope doc-viewer-object-array"></i>
<div class="doc-viewer-value" ng-bind-html="typeof(formatted[field]) === 'undefined' ? hit[field] : formatted[field] | trustAsHtml"></div>
</td>
</tr>
</tbody>
</table>
33 changes: 33 additions & 0 deletions src/ui/public/doc_viewer/viewers/table.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
import _ from 'lodash';
import docTableDetailViewsRegistry from 'ui/registry/doc_table_detail_views';

import tableHtml from './table.html';

docTableDetailViewsRegistry.register(function () {
return {
title: 'Table',
order: 10,
template: tableHtml,
shouldShow: () => true,
controller: function ($scope) {
$scope.indexPattern = $scope.scope.indexPattern;
$scope.hit = $scope.scope.hit;
$scope.columns = $scope.scope.columns;
$scope.filter = $scope.scope.filter;

$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.showArrayInObjectsWarning = function (row, field) {
var value = $scope.flattened[field];
return _.isArray(value) && typeof value[0] === 'object';
};
}
};
});
7 changes: 7 additions & 0 deletions src/ui/public/registry/doc_table_detail_views.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
define(function (require) {
return require('ui/registry/_registry')({
name: 'docTableDetailViews',
index: ['title'],
order: ['order']
Copy link
Contributor

Choose a reason for hiding this comment

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

In order to implement the defaults I mentioned above you will either want to create a DocView class or add a constructor here that can modify the registry when it is injected into the application. I personally prefer the class approach, but it's up to you.

return require('ui/registry/_registry')({
  name: 'docTableDetailViews',
  constructor() {
    this.forEach(docView => {
      docView.shouldShow = docView.shouldShow || _.constant(true)
    })
  }
});

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I went with the constructor approach. I also added a new field called "name". It defaults to title but is meant for the case where title has quotes or other strange characters that might mess up the docViews[title] code. So, now you can specify a name in that case.

});
});
5 changes: 3 additions & 2 deletions src/ui/public/render_directive/render_directive.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,8 @@ module.directive('renderDirective', function () {
return {
restrict: 'E',
scope: {
'definition': '='
'definition': '=',
'scope': '=?'
Copy link
Contributor

Choose a reason for hiding this comment

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

😔 not sure how we can make this better but I hope we can come up with something. Referring to $scope.scope internally is gross.

Copy link
Contributor

Choose a reason for hiding this comment

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

What if the definition included a scope definition? We would have to implement that in render_directive, but that sounds like fun to me.

definition = {
  controller...
  controllerAs...
  scope: {
    hit: '=',
    indexPattern: '=',
    onSave: '&'
  }
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I took a stab at this in #6115, mind trying it out?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried it out. It works but feels a little awkward. I have to define the attributes in both the definition and in the render-directive html.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've updated the pull request and you can see how I used it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, I understand how that might be a bit awkward but I see it as documenting what you pass in and what you expect to be passed in.

},
template: function ($el, $attrs) {
return $el.html();
Expand All @@ -27,4 +28,4 @@ module.directive('renderDirective', function () {
}
}
};
});
});
1 change: 1 addition & 0 deletions src/ui/ui_exports.js
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ class UiExports {
case 'chromeNavControls':
case 'navbarExtensions':
case 'settingsSections':
case 'docTableDetailViews':
return (plugin, spec) => {
this.aliases[type] = _.union(this.aliases[type] || [], spec);
};
Expand Down