Refactoring to put browser specific code in a new class #3718

etimberg · 2016-12-19T01:14:32Z

First revision of the implementation of https://simonbrunel.gitbooks.io/chartjs-proposals/content/Platform.html#event-management

The BrowserPlatform class is attached to each chart to handle browser specific logic. This can be replaced on a different platform. BrowserPlatform implements IPlatform (the general interface that all platform implementations must support). Chart.Platform is the constructor for the platform object that is attached to the chart instance.

Plugins are notified about the event using the onEvent call. The legend plugin was converted to use onEvent instead of the older private handleEvent method.
Wrote test to check that plugins are notified about events

etimberg · 2016-12-19T01:16:28Z

I'm not sure if it's better to have a single onEvent for plugins or if it's better to have one for each internal event type (ie. onMouseMove, onClick, etc)

…orm. BrowserPlatform implements IPlatform. Chart.Platform is the constructor for the platform object that is attached to the chart instance. Plugins are notified about the event using the `onEvent` call. The legend plugin was converted to use onEvent instead of the older private `handleEvent` method. Wrote test to check that plugins are notified about events

simonbrunel

Looks like I can drop my local work on the platform stuff! Your version looks good, the main difference is that Chart.Platform is a static class in my case, no inheritance, simply overrides (requiring platform.dom.js defines Chart.Platform.* implementation for browser). I don't really see advantages to have a platform instance for each charts, e.g. acquiring a context could be Chart.Platform.acquireContext(item, config). Anyway, great job!

simonbrunel · 2016-12-19T22:33:42Z

src/core/core.platform.js

+
+// Core.Platform abstracts away browser specific APIs from the chart
+module.exports = function(Chart) {
+	var platform = Chart.corePlatform = {};


Can we simply call it Chart.platform?

Yeah, that was actually my first thought before I made it more OO like. I'll move to a static class in Chart.Platform

simonbrunel · 2016-12-19T22:34:42Z

src/core/core.platform.js

+	 * @param config {ChartOptions} the chart options
+	 */
+	/**
+	 * @method IPlatform#releaseCanvas


Should be releaseContext instead, canvas should be totally abstracted from the platform API.

sounds good.

should this also be changed to take a CanvasRenderingContext2D as the parameter? Right now it's the canvas dom node

Yeah, releaseContext should take the context2d as parameter (the one that acquireContext returns).

simonbrunel · 2016-12-19T22:36:30Z

src/platforms/platform.browser.js

+	 * @param chartController {Core.Controller} the main chart controller
+	 */
+	function BrowserPlatform(chartController) {
+		this.chartController = chartController;


this.controller ?

simonbrunel · 2016-12-20T09:52:39Z

src/core/core.platform.js

+	 * @name Core.platform.events
+	 * @type Map<String, String>
+	 */
+	platform.events = {


Do we really need that map, could simply directly use strings values?

I think we can directly just use string variables.

What I mean is I don't think we even need variables, just a doc comment with all Chart.js event names, then directly use strings into the code (e.g. if (e.type === 'mouseenter') { ... } and in the BrowserPlatform map: pointerenter: 'mouseenter').

simonbrunel · 2016-12-20T09:55:38Z

src/platforms/platform.browser.js

+	BrowserPlatform.prototype.createEvent = function(e) {
+		var chart = this.chartController.chart;
+		var relativePosition = helpers.getRelativePosition(e, chart);
+		var chartEvent = {


Can avoid the local variable (return { ... })

simonbrunel · 2016-12-20T09:57:00Z

test/core.controller.tests.js

 		});
 	});
+
+	describe('event handling', function() {


All platform related tests should be moved to test/platform.browser.tests.js

Edit: wrong file, should be platform.browser.tests.js

do you think the responsive tests should move here as well? or just the acquireContext, releaseContext and event tests?

I'm not sure what should be moved exactly, I guess everything that touch the DOM and canvas element.

simonbrunel · 2016-12-20T14:29:22Z

src/core/core.platform.js

+	 */
+	/**
+	 * @method IPlatform#acquireContext
+	 * @param item {CanvasRenderingContext2D|HTMLCanvasElement} the context or canvas to use


Item type should be Object since on other platforms it might be anything (e.g. in QML, it's an QML Item). Also missing the doc for the return type: @returns A context2d instance implementing the W3C Canvas 2D Context API standard

…m.dom and move associated tests into platform.dom.tests. Chart.platform is now a static class instead of a constructable object.

simonbrunel · 2016-12-20T20:32:45Z

src/chart.js

 require('./core/core.tooltip')(Chart);

+// By default, we only load the browser platform.
+Chart.platform = require('./platforms/platform.dom')(Chart);


Should it be Chart.Platform or Chart.platform? I thought namespaces was all lowercase and classes with a capital.

I used Chart.platform since it's not a constructor

Works for me, just thinking that it might have been a good idea to have some kind of naming rules for namespace and class names: I used to see namespaces in lowercase and class capitalized (whatever static or not).

ok, I'm fine with that. I'll change this to be Platform since it's a class

Refactoring to put browser specific code in a new class, BrowserPlatform. BrowserPlatform implements IPlatform. Chart.Platform is the constructor for the platform object that is attached to the chart instance. Plugins are notified about the event using the `onEvent` call. The legend plugin was converted to use onEvent instead of the older private `handleEvent` method. Wrote test to check that plugins are notified about events

etimberg added this to the Version 2.5 milestone Dec 19, 2016

etimberg requested review from panzarino and simonbrunel December 19, 2016 01:14

etimberg force-pushed the plugin-events branch from 449a060 to d837bce Compare December 19, 2016 01:16

etimberg force-pushed the plugin-events branch from d837bce to 7d8afe8 Compare December 19, 2016 01:25

etimberg force-pushed the plugin-events branch from 7d8afe8 to 41f56fb Compare December 19, 2016 01:46

simonbrunel reviewed Dec 20, 2016

View reviewed changes

Updates per code review comments. Renamed platform.browser to platfor…

dc59ea2

…m.dom and move associated tests into platform.dom.tests. Chart.platform is now a static class instead of a constructable object.

simonbrunel reviewed Dec 20, 2016

View reviewed changes

simonbrunel approved these changes Dec 21, 2016

View reviewed changes

etimberg merged commit ecc35c5 into master Dec 21, 2016

etimberg deleted the plugin-events branch December 21, 2016 15:22

This was referenced Dec 21, 2016

HTML Table within Tooltip callback #3734

Closed

Cursor on horizontal bar #3737

Closed

[FEATURE] Add tooltips which draw one horizontal line which follow mouse cursor to help #3744

Closed

This was referenced Dec 30, 2016

Enhancement: Native support for events #2904

Closed

[v2] Click & Tick #2403

Closed

etimberg mentioned this pull request Jan 7, 2017

How to apply "cursor: pointer" css #3539

Closed

simonbrunel mentioned this pull request Jan 14, 2017

Platform event API abstraction #3795

Merged

etimberg mentioned this pull request Jan 18, 2017

Vertical Line Mousemove #3803

Closed

compwright mentioned this pull request Nov 1, 2017

Don’t propagate clicks that are handled internally #3679

Closed

Refactoring to put browser specific code in a new class #3718

Refactoring to put browser specific code in a new class #3718

Uh oh!

Conversation

etimberg commented Dec 19, 2016

Uh oh!

etimberg commented Dec 19, 2016

Uh oh!

simonbrunel left a comment • 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!

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!

simonbrunel Dec 20, 2016 • 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!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

simonbrunel left a comment •

edited

Loading

simonbrunel Dec 20, 2016 •

edited

Loading