Skip to content

Commit

Permalink
[5.x]: Trigger renderComplete on DOM instead of the Vis (#9176) (#9203)
Browse files Browse the repository at this point in the history
Backports PR #9183

**Commit 1:**
BACKPORT: Trigger renderComplete on DOM instead of the Vis (#9176)

* correctly use implementsRenderComplete from vis config

* set implementsRenderComplete on all vis types

* emit on DOM instead of the vis

* vis doesn't need to be an EventEmitter

* listen for renderComplete on visualize

set and update element attributes as the event(s) fire, and indicate if
they ever will

* [vislib/handler] fall through to lower return

* [vislibRenderbot/tests] reduce expected call count

* [vis/tests] add $element to test injectors

* [markdownVis] fix test

* Original sha: 7190260
* Authored by Joe Fleming <joe.fleming@gmail.com> on 2016-11-22T16:49:16Z
  • Loading branch information
elastic-jasper authored and epixa committed Nov 23, 2016
1 parent 867ff10 commit 67db8dd
Show file tree
Hide file tree
Showing 19 changed files with 53 additions and 45 deletions.
1 change: 1 addition & 0 deletions src/core_plugins/kbn_vislib_vis_types/public/area.js
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@ export default function HistogramVisType(Private) {
modes: ['stacked', 'overlap', 'percentage', 'wiggle', 'silhouette'],
editor: areaTemplate
},
implementsRenderComplete: true,
schemas: new Schemas([
{
group: 'metrics',
Expand Down
1 change: 1 addition & 0 deletions src/core_plugins/kbn_vislib_vis_types/public/histogram.js
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ export default function HistogramVisType(Private) {
modes: ['stacked', 'percentage', 'grouped'],
editor: histogramTemplate
},
implementsRenderComplete: true,
schemas: new Schemas([
{
group: 'metrics',
Expand Down
1 change: 1 addition & 0 deletions src/core_plugins/kbn_vislib_vis_types/public/line.js
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@ export default function HistogramVisType(Private) {
scales: ['linear', 'log', 'square root'],
editor: lineTemplate
},
implementsRenderComplete: true,
schemas: new Schemas([
{
group: 'metrics',
Expand Down
1 change: 1 addition & 0 deletions src/core_plugins/kbn_vislib_vis_types/public/pie.js
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,7 @@ export default function HistogramVisType(Private) {
},
responseConverter: false,
hierarchicalData: true,
implementsRenderComplete: true,
schemas: new Schemas([
{
group: 'metrics',
Expand Down
1 change: 1 addition & 0 deletions src/core_plugins/kbn_vislib_vis_types/public/tile_map.js
Original file line number Diff line number Diff line change
Expand Up @@ -81,6 +81,7 @@ export default function TileMapVisType(Private, getAppState, courier, config) {
}
},
responseConverter: geoJsonConverter,
implementsRenderComplete: true,
schemas: new Schemas([
{
group: 'metrics',
Expand Down
Original file line number Diff line number Diff line change
@@ -1,24 +1,24 @@
import ngMock from 'ng_mock';
import expect from 'expect.js';
import $ from 'jquery';

describe('markdown vis controller', function () {
let $scope;
let $el;

beforeEach(ngMock.module('kibana/markdown_vis'));
beforeEach(ngMock.inject(function ($rootScope, $controller) {
$scope = $rootScope.$new();
$scope.vis = {
emit: function () {}
};
$controller('KbnMarkdownVisController', {$scope: $scope});
const $element = $('<div>');
$controller('KbnMarkdownVisController', { $scope, $element });
$scope.$digest();
}));

it('should set html from markdown params', function () {
expect($scope).to.not.have.property('html');
$scope.vis.params = {
markdown: 'This is a test of the [markdown](http://daringfireball.net/projects/markdown) vis.'
$scope.vis = {
params: {
markdown: 'This is a test of the [markdown](http://daringfireball.net/projects/markdown) vis.'
}
};
$scope.$digest();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -9,11 +9,11 @@ marked.setOptions({


const module = uiModules.get('kibana/markdown_vis', ['kibana', 'ngSanitize']);
module.controller('KbnMarkdownVisController', function ($scope) {
module.controller('KbnMarkdownVisController', function ($scope, $element) {
$scope.$watch('vis.params.markdown', function (html) {
if (html) {
$scope.html = marked(html);
}
$scope.vis.emit('renderComplete');
$element.trigger('renderComplete');
});
});
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import ngMock from 'ng_mock';
import expect from 'expect.js';
import $ from 'jquery';

describe('metric vis', function () {
let $scope;
Expand All @@ -11,7 +12,8 @@ describe('metric vis', function () {
beforeEach(ngMock.module('kibana/metric_vis'));
beforeEach(ngMock.inject(function ($rootScope, $controller) {
$scope = $rootScope.$new();
$controller('KbnMetricVisController', {$scope: $scope});
const $element = $('<div>');
$controller('KbnMetricVisController', { $scope, $element });
$scope.$digest();
}));

Expand Down
4 changes: 2 additions & 2 deletions src/core_plugins/metric_vis/public/metric_vis_controller.js
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@ import uiModules from 'ui/modules';
// didn't already
const module = uiModules.get('kibana/metric_vis', ['kibana']);

module.controller('KbnMetricVisController', function ($scope, Private) {
module.controller('KbnMetricVisController', function ($scope, $element, Private) {
const tabifyAggResponse = Private(AggResponseTabifyTabifyProvider);

const metrics = $scope.metrics = [];
Expand Down Expand Up @@ -34,7 +34,7 @@ module.controller('KbnMetricVisController', function ($scope, Private) {
if (resp) {
metrics.length = 0;
$scope.processTableGroups(tabifyAggResponse($scope.vis, resp));
$scope.vis.emit('renderComplete');
$element.trigger('renderComplete');
}
});
});
4 changes: 2 additions & 2 deletions src/core_plugins/table_vis/public/table_vis_controller.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ const module = uiModules.get('kibana/table_vis', ['kibana']);

// add a controller to tha module, which will transform the esResponse into a
// tabular format that we can pass to the table directive
module.controller('KbnTableVisController', function ($scope, Private) {
module.controller('KbnTableVisController', function ($scope, $element, Private) {
const tabifyAggResponse = Private(AggResponseTabifyTabifyProvider);

var uiStateSort = ($scope.uiState) ? $scope.uiState.get('vis.params.sort') : {};
Expand Down Expand Up @@ -44,7 +44,7 @@ module.controller('KbnTableVisController', function ($scope, Private) {
return table.rows.length > 0;
});

$scope.vis.emit('renderComplete');
$element.trigger('renderComplete');
}

$scope.hasSomeRows = hasSomeRows;
Expand Down
13 changes: 11 additions & 2 deletions src/core_plugins/timelion/public/vis/timelion_vis_controller.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,16 @@ define(function (require) {

var _ = require('lodash');
var module = require('ui/modules').get('kibana/timelion_vis', ['kibana']);
module.controller('TimelionVisController', function ($scope, Private, Notifier, $http, $rootScope, timefilter, getAppState) {
module.controller('TimelionVisController', function (
$scope,
$element,
Private,
Notifier,
$http,
$rootScope,
timefilter,
getAppState
) {
var queryFilter = Private(require('ui/filter_bar/query_filter'));
var timezone = Private(require('plugins/timelion/services/timezone'))();
var dashboardContext = Private(require('plugins/timelion/services/dashboard_context'));
Expand Down Expand Up @@ -61,7 +70,7 @@ define(function (require) {

$scope.$on('renderComplete', event => {
event.stopPropagation();
$scope.vis.emit('renderComplete');
$element.trigger('renderComplete');
});

});
Expand Down
2 changes: 1 addition & 1 deletion src/ui/public/template_vis_type/template_vis_type.js
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,7 @@ export default function TemplateVisTypeFactory(Private) {
let TemplateRenderbot = Private(TemplateVisTypeTemplateRenderbotProvider);

_.class(TemplateVisType).inherits(VisType);
function TemplateVisType(opts) {
function TemplateVisType(opts = {}) {
TemplateVisType.Super.call(this, opts);

this.template = opts.template;
Expand Down
13 changes: 0 additions & 13 deletions src/ui/public/vis/vis.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,14 +20,12 @@ export default function VisFactory(Notifier, Private) {
let visTypes = Private(RegistryVisTypesProvider);
let AggConfigs = Private(VisAggConfigsProvider);
const PersistedState = Private(PersistedStateProvider);
const EventEmitter = Private(EventsProvider);

let notify = new Notifier({
location: 'Vis'
});

function Vis(indexPattern, state, uiState) {
EventEmitter.call(this);
state = state || {};

if (_.isString(state)) {
Expand All @@ -40,8 +38,6 @@ export default function VisFactory(Notifier, Private) {

this.setState(state);
this.setUiState(uiState);

this.on('renderComplete', () => this._renderComplete = true);
}

Vis.convertOldState = function (type, oldState) {
Expand Down Expand Up @@ -85,7 +81,6 @@ export default function VisFactory(Notifier, Private) {
};
};

Vis.prototype = Object.create(EventEmitter.prototype);
Vis.prototype.type = 'histogram';

Vis.prototype.setState = function (state) {
Expand Down Expand Up @@ -173,14 +168,6 @@ export default function VisFactory(Notifier, Private) {
return this.type.implementsRenderComplete;
};

Vis.prototype.onRenderComplete = function (cb) {
this.on('renderComplete', cb);
if (this._renderComplete) {
// guarantees that caller receives event in case visualization already finished rendering
this.emit('renderComplete');
}
};

/**
* Currently this is only used to extract map-specific information
* (e.g. mapZoom, mapCenter).
Expand Down
2 changes: 1 addition & 1 deletion src/ui/public/vis/vis_type.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ export default function VisTypeFactory(Private) {
this.schemas = opts.schemas || new VisTypeSchemas();
this.params = opts.params || {};
this.requiresSearch = opts.requiresSearch == null ? true : opts.requiresSearch; // Default to true unless otherwise specified
this.implementsRenderComplete = false;
this.implementsRenderComplete = opts.implementsRenderComplete || false;
}

VisType.prototype.createRenderbot = function (vis, $el, uiState) {
Expand Down
9 changes: 5 additions & 4 deletions src/ui/public/vislib/lib/handler/handler.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
import d3 from 'd3';
import _ from 'lodash';
import $ from 'jquery';
import errors from 'ui/errors';
import Binder from 'ui/binder';
import VislibLibDataProvider from 'ui/vislib/lib/data';
Expand Down Expand Up @@ -126,8 +127,8 @@ export default function HandlerBaseClass(Private) {
binder.on(chart.events, 'rendered', () => {
loadedCount++;
if (loadedCount === chartSelection.length) {
// events from all charts are propagated to vis, we only need to fire renderComplete on one (first)
charts[0].events.emit('renderComplete');
// events from all charts are propagated to vis, we only need to fire renderComplete once they all finish
$(self.el).trigger('renderComplete');
}
});

Expand Down Expand Up @@ -184,11 +185,11 @@ export default function HandlerBaseClass(Private) {
.append('h4').text(message);

div.append('div').attr('class', 'item bottom');
return div;
} else {
div.append('h4').text(message);
}
this.vis.emit('renderComplete');

$(this.el).trigger('renderComplete');
return div;
};

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,7 @@ describe('renderbot', function exportWrapper() {
});

it('should attach listeners and set vislibVis', function () {
expect(listenerSpy.callCount).to.be(4);
expect(listenerSpy.callCount).to.be(3);
expect(listenerSpy.calledWith('test', _.noop)).to.be(true);
expect(renderbot.vislibVis).to.be.a(Vis);
});
Expand Down
4 changes: 0 additions & 4 deletions src/ui/public/vislib_vis_type/vislib_renderbot.js
Original file line number Diff line number Diff line change
Expand Up @@ -24,10 +24,6 @@ module.exports = function VislibRenderbotFactory(Private, $injector) {
this.vislibVis.on(event, listener);
});

this.vislibVis.on('renderComplete', () => {
this.vis.emit('renderComplete');
});

if (this.chartData) {
this.vislibVis.render(this.chartData, this.uiState);
}
Expand Down
5 changes: 1 addition & 4 deletions src/ui/public/vislib_vis_type/vislib_vis_type.js
Original file line number Diff line number Diff line change
Expand Up @@ -16,17 +16,14 @@ export default function VislibVisTypeFactory(Private) {


_.class(VislibVisType).inherits(VisType);
function VislibVisType(opts) {
opts = opts || {};

function VislibVisType(opts = {}) {
VislibVisType.Super.call(this, opts);

if (this.responseConverter == null) {
this.responseConverter = pointSeries;
}

this.listeners = opts.listeners || {};
this.implementsRenderComplete = true;
}

VislibVisType.prototype.createRenderbot = function (vis, $el, uiState) {
Expand Down
13 changes: 12 additions & 1 deletion src/ui/public/visualize/visualize.js
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,16 @@ uiModules
return legendPositionToVisContainerClassMap[$scope.vis.params.legendPosition];
};

if ($scope.vis.implementsRenderComplete()) {
$el.attr('has-render-count', 'true');
let renderCount = 0;
$el.on('renderComplete', () => {
$el.attr('render-count', ++renderCount);
});
} else {
$el.attr('has-render-count', 'false');
}

$scope.spy = {};
$scope.spy.mode = ($scope.uiState) ? $scope.uiState.get('spy.mode', {}) : {};

Expand Down Expand Up @@ -168,7 +178,7 @@ uiModules
}).catch(notify.fatal);

searchSource.onError(e => {
$scope.vis.emit('renderComplete');
$el.trigger('renderComplete');
if (isTermSizeZeroError(e)) {
return notify.error(
`Your visualization ('${$scope.vis.title}') has an error: it has a term ` +
Expand All @@ -194,6 +204,7 @@ uiModules

$scope.$on('$destroy', function () {
if ($scope.renderbot) {
$el.off('renderComplete');
$scope.renderbot.destroy();
}
});
Expand Down

0 comments on commit 67db8dd

Please sign in to comment.