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

Prevent charts from rendering twice #8371

Merged
merged 3 commits into from
Sep 22, 2016

Conversation

ppisljar
Copy link
Member

Charts in kibana (pie and point charts) render twice. There are two problems:

  • resize checker is checking for new size and rerenders the chart in that case. when we render the chart for the first time its size changes ... resize checker starts rerender just after we render it the first time
  • even if we fix the first problem, still the size of chart changes after legend is added to the view. to fix that we first need to render legend and second the actual chart

@ppisljar ppisljar added bug Fixes for quality problems that affect the customer experience review v5.0.0 labels Sep 20, 2016
@thomasneirynck
Copy link
Contributor

@ppisljar Is this the same issue as #7945/#684?

@ppisljar
Copy link
Member Author

ppisljar commented Sep 20, 2016

no @thomasneirynck it is related, but not the same. everytime visualization is rendered its done so twice. first normal render then resize_checker finds out that the size of chart changed (becase before divs height was 0 and now its actual chart height or because legend appeared) and rerenders it again.

it doesn't depend on changing any options (the other two issues seem to be problems when chart is rerendered after changing option when that is not needed )

@@ -46,7 +46,10 @@ module.exports = function VislibRenderbotFactory(Private) {
VislibRenderbot.prototype.buildChartData = buildChartData;
VislibRenderbot.prototype.render = function (esResponse) {
this.chartData = this.buildChartData(esResponse);
this.vislibVis.render(this.chartData, this.uiState);
// to allow legend to render first
setTimeout(() => {
Copy link
Member Author

Choose a reason for hiding this comment

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

i don't really like this (and its breaking tests). would returning a promise here be a good idea ?

Copy link
Contributor

@Bargs Bargs Sep 20, 2016

Choose a reason for hiding this comment

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

Could you create a "legend rendered" event and listen for it? Or if render directly modifies the DOM, perhaps you could listen to for the DOM modification event that adds the legend? I've never really looked at the Renderbot code, so I'm probably not a ton of help here.

Copy link
Member Author

Choose a reason for hiding this comment

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

legend is waiting for renderbot.chartData to be available. once thats done it'll render in the next digest cycle so its sufficient for renderbot to just wait here after it makes chartData available

@Bargs Bargs self-assigned this Sep 20, 2016
@Bargs Bargs removed the review label Sep 20, 2016
@ppisljar ppisljar assigned spalger and unassigned Bargs Sep 21, 2016
expect(renderStub.callCount).to.be(1);
expect(buildStub.callCount).to.be(1);
expect(renderStub.firstCall.args[0]).to.be(football);
renderbot.render('flat data', persistedState).then(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

you need to return the promise here, or the assertions won't fail the test.

this.vislibVis.render(this.chartData, this.uiState);
// to allow legend to render first (wait for angular digest cycle to complete)
return new Promise((resolve, reject) => {
setTimeout(() => {
Copy link
Contributor

Choose a reason for hiding this comment

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

This should use $scope.$evalAsync() so that we can be sure that angular has time to finish it's digest, and so that it runs one after we are done.

Copy link
Member Author

Choose a reason for hiding this comment

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

renderbot is non-angular library so it doesnt use scope ... do you think its a good idea to introduce it ? i kinda like it to be a separate thing.

@@ -64,8 +64,12 @@ export default function VisFactory(Private) {
uiState.on('change', this._uiStateChangeHandler = () => this.render(this.data, this.uiState));
}

this.resizeChecker.stopSchedule();
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a pretty serious collection of steps. What do you think about naming it something and adding it as a method to the ResizeChecker.

Maybe something like:

ResizeChecker.prototype.changeWithoutTriggering = function(block) {
  // prevent checking while the block runs
  this.stopSchedule();

  // run the external code that will cause the size to change
  block();

  // save the new size, without emitting a change, and restart the schedule
  this.saveSize();
  this.saveDirty(false);
  this.continueSchedule();
};

Which would be used as:

this.resizeChecker.changeWithoutTriggering(() => {
  this.handler = handlerTypes[chartType](this) || handlerTypes.column(this);
  this._runOnHandler('render');
})

Copy link
Member Author

Choose a reason for hiding this comment

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

what do you think about adding function to the vislib class ? (take a look at latest commit) ?

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 it

@thomasneirynck
Copy link
Contributor

LGTM. I like the improvement!

@ppisljar ppisljar merged commit 0b8ff40 into elastic:master Sep 22, 2016
ppisljar added a commit that referenced this pull request Sep 22, 2016
---------

**Commit 1:**
fixing charts to not render twice

* Original sha: f7f0f12
* Authored by ppisljar <[email protected]> on 2016-09-20T08:55:55Z

**Commit 2:**
fixing tests

* Original sha: 40234e8
* Authored by ppisljar <[email protected]> on 2016-09-21T07:21:01Z

**Commit 3:**
fixing based on Spencers comments

* Original sha: cdad772
* Authored by ppisljar <[email protected]> on 2016-09-21T19:41:57Z
ppisljar added a commit that referenced this pull request Sep 22, 2016
---------

**Commit 1:**
fixing charts to not render twice

* Original sha: f7f0f12
* Authored by ppisljar <[email protected]> on 2016-09-20T08:55:55Z

**Commit 2:**
fixing tests

* Original sha: 40234e8
* Authored by ppisljar <[email protected]> on 2016-09-21T07:21:01Z

**Commit 3:**
fixing based on Spencers comments

* Original sha: cdad772
* Authored by ppisljar <[email protected]> on 2016-09-21T19:41:57Z
(cherry picked from commit 61d487c)
ppisljar added a commit that referenced this pull request Sep 22, 2016
ppisljar added a commit that referenced this pull request Sep 22, 2016
@epixa epixa added v5.0.0-rc1 and removed v5.0.0 labels Oct 7, 2016
@epixa epixa added the v5.0.0 label Oct 7, 2016
@epixa epixa changed the title fixing charts to not render twice Prevent charts from rendering twice Oct 9, 2016
@tbragin tbragin added the Feature:Visualizations Generic visualization features (in case no more specific feature label is available) label Dec 16, 2016
airow pushed a commit to airow/kibana that referenced this pull request Feb 16, 2017
---------

**Commit 1:**
fixing charts to not render twice

* Original sha: d386bf04d6899d2d2c884504fd931342e59668be [formerly f7f0f12]
* Authored by ppisljar <[email protected]> on 2016-09-20T08:55:55Z

**Commit 2:**
fixing tests

* Original sha: 9eb6e1c53602065bc5b5b39c278876852c1cb672 [formerly 40234e8]
* Authored by ppisljar <[email protected]> on 2016-09-21T07:21:01Z

**Commit 3:**
fixing based on Spencers comments

* Original sha: a876c212643aad14444e514a4b38fe67b73ee1d9 [formerly cdad772]
* Authored by ppisljar <[email protected]> on 2016-09-21T19:41:57Z


Former-commit-id: 61d487c
airow pushed a commit to airow/kibana that referenced this pull request Feb 16, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Fixes for quality problems that affect the customer experience Feature:Visualizations Generic visualization features (in case no more specific feature label is available) review v5.0.0-rc1 v5.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants