Add pollingInterval option to Charts #704

julien · 2018-03-07T12:38:25Z

No description provided.

Fixes #544

coveralls · 2018-03-07T12:46:31Z

Pull Request Test Coverage Report for Build 470

32 of 38 (84.21%) changed or added relevant lines in 4 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.08%) to 81.908%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
packages/clay-charts/src/ChartBase.js	12	14	85.71%
packages/clay-charts/src/Geomap.js	1	5	20.0%

Totals
Change from base Build 459:	0.08%
Covered Lines:	4511
Relevant Lines:	4774

💛 - Coveralls

jbalsas · 2018-03-07T13:01:54Z

packages/clay-charts/src/Chart.js

@@ -11,7 +12,8 @@ import templates from './Chart.soy.js';
 class Chart extends Component {}

 Object.assign(Chart.prototype, ChartBase);
-Chart.STATE = ChartBase.STATE;
+Object.assign(Chart.prototype, DataComponent);


Why not Object.assign(Chart.prototype, DataComponent, ChartBase); all in one line?

jbalsas · 2018-03-07T13:02:35Z

packages/clay-charts/src/Chart.js

@@ -11,7 +12,8 @@ import templates from './Chart.soy.js';
 class Chart extends Component {}

 Object.assign(Chart.prototype, ChartBase);
-Chart.STATE = ChartBase.STATE;
+Object.assign(Chart.prototype, DataComponent);
+Chart.STATE = Object.assign(ChartBase.STATE, DataComponent.STATE);


Should this rather be Object.assign({}, ChartBase.STATE, DataComponent.STATE)? Otherwise, aren't we mutating ChartBase.STATE?

Yes we are, good catch thanks

jbalsas · 2018-03-07T13:03:43Z

packages/clay-charts/src/ChartBase.js

@@ -77,6 +76,7 @@ const ChartBase = {
 		}

 		this._resolveData(this.data).then(data => {
+			this._setupPolling();


This should go to DataComponent, as ChartBase has no knowledge at all about polling

Maybe this could go inside the _resolveData method, right before resolving the Promise?

jbalsas · 2018-03-07T13:03:53Z

packages/clay-charts/src/ChartBase.js

@@ -103,6 +103,10 @@ const ChartBase = {
 			return;
 		}

+		if (this._pollingInterval) {
+			clearInterval(this._pollingInterval);
+		}


This should go to DataComponent, as ChartBase has no knowledge at all about polling

Ok, but ChartBase (or Chart if you prefer) will have to call this method defined in DataComponent since it can't be called disposed. Do you agree?

jbalsas · 2018-03-07T13:04:14Z

packages/clay-charts/src/Geomap.js

@@ -19,6 +19,8 @@ class Geomap extends Component {
 			return;
 		}

+		this._setupPolling();


This should go to DataComponent, as ChartBase has no knowledge at all about polling

jbalsas · 2018-03-07T13:04:21Z

packages/clay-charts/src/Geomap.js

@@ -72,6 +74,10 @@ class Geomap extends Component {
 			return;
 		}

+		if (this._pollingInterval) {
+			clearTimeout(this._pollingInterval);
+		}


This should go to DataComponent, as ChartBase has no knowledge at all about polling

jbalsas · 2018-03-07T13:04:35Z

packages/clay-charts/src/Geomap.js

 };

+Object.assign(Geomap.prototype, DataComponent);
+Object.assign(Geomap.STATE, DataComponent.STATE);


Same as before?

Here, I'm mutating Geomap.prototype which is what I want and doing the same with Geomap.STATE which is another object so I don't really want to mix the two, but I'm not modifying DataComponent or DataComponent.STATE, did I miss anything?

It probably makes more sense to have Geomap extend from DataComponent (and probably do the same for Chart not ChartBase) since itself extends Component, and just copy the STATE over. What do you think?

jbalsas · 2018-03-08T08:24:23Z

packages/clay-charts/src/Chart.js


 Object.assign(Chart.prototype, ChartBase);
-Chart.STATE = ChartBase.STATE;
+Chart.STATE = Object.assign({}, ChartBase.STATE, DataComponent.STATE);


I think that we probably don't need to merge DataComponent.STATE here, since metal will pick it up from the class chain. Could you double check?

Yes you're right, this is not needed. Thanks.

jbalsas · 2018-03-08T08:26:05Z

packages/clay-charts/src/Geomap.js

 };

+Object.assign(Geomap.STATE, DataComponent.STATE);


Since we already extend DataComponent, is this really necessary?

No, you're right.

jbalsas · 2018-03-12T11:56:21Z

packages/clay-charts/src/ChartBase.js

@@ -103,6 +103,8 @@ const ChartBase = {
 			return;
 		}

+		this.cleanup();


Do we really need to add this additional lifecycle method? Wouldn't it be enough to just implement disposed() in DataComponent

Yes, you're right; that's the thing to do, thanks.

Changes applied

Add polling_interval option to Charts

caef9ca

Fixes #544

julien changed the title ~~Add polling_interval option to Charts~~ Add pollingInterval option to Charts Mar 7, 2018

julien requested a review from jbalsas March 7, 2018 12:38

jbalsas suggested changes Mar 7, 2018

View reviewed changes

Apply requested changes and add test

10d84e5

jbalsas previously requested changes Mar 8, 2018

View reviewed changes

Apply requested changes

26492c5

jbalsas reviewed Mar 12, 2018

View reviewed changes

Apply requested changes

78eb406

jbalsas merged commit 0b741aa into liferay:develop Mar 12, 2018

julien deleted the issue-544 branch March 13, 2018 08:19

		};

		Object.assign(Geomap.STATE, DataComponent.STATE);

Add pollingInterval option to Charts #704

Add pollingInterval option to Charts #704

Uh oh!

Conversation

julien commented Mar 7, 2018

Uh oh!

coveralls commented Mar 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Pull Request Test Coverage Report for Build 470

💛 - Coveralls

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

julien Mar 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

julien Mar 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

coveralls commented Mar 7, 2018 •

edited

Loading

julien Mar 7, 2018 •

edited

Loading

julien Mar 7, 2018 •

edited

Loading