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.

Sign up for GitHub

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

Jump to bottom

WIP: add registerController for controllers #7407

Closed
wants to merge 8 commits into from
Closed

WIP: add registerController for controllers #7407

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
a0935f7
add registerController for controllers
sgratzl May 23, 2020
5c8846e
remove not needed import
sgratzl May 23, 2020
b748f43
fix linting
sgratzl May 23, 2020
093034a
add type hints any to avoid 'conflicts'
sgratzl May 23, 2020
fbd2a7b
fix test
sgratzl May 23, 2020
67ed9a0
rename to beforeRegister
sgratzl May 23, 2020
21bf9b4
integrate feedback
sgratzl May 24, 2020
083b662
fix linting error
sgratzl May 24, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
38 changes: 22 additions & 16 deletions src/controllers/controller.bar.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,13 @@
import DatasetController from '../core/core.datasetController';
import defaults from '../core/core.defaults';
import {Rectangle} from '../elements/index';
import {clipArea, unclipArea} from '../helpers/helpers.canvas';
import {isArray, isNullOrUndef, valueOrDefault} from '../helpers/helpers.core';
import {_limitValue, sign} from '../helpers/helpers.math';

defaults.set('bar', {
/**
* @type {any}
*/
const defaults = {
benmccann marked this conversation as resolved.
Show resolved Hide resolved
hover: {
mode: 'index'
},
Expand Down Expand Up @@ -34,7 +36,7 @@ defaults.set('bar', {
beginAtZero: true,
}
}
});
};

/**
* Computes the "optimal" sample size to maintain bars equally sized while preventing overlap.
Expand Down Expand Up @@ -496,16 +498,20 @@ export default class BarController extends DatasetController {

}

BarController.prototype.dataElementType = Rectangle;

BarController.prototype.dataElementOptions = [
'backgroundColor',
'borderColor',
'borderSkipped',
'borderWidth',
'barPercentage',
'barThickness',
'categoryPercentage',
'maxBarThickness',
'minBarLength'
];
BarController.id = 'bar';
BarController.defaults = defaults;
BarController.preRegister = () => {
Copy link
Member

Choose a reason for hiding this comment

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

Could use beforeRegister and afterRegister? That way we'd match the format of other lifecycle calls

Copy link
Member

Choose a reason for hiding this comment

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

I agree tho those names and we could have those hooks, but I think we should not expect anything to happen in beforeRegister/afterRegister. Maybe we can come up with a better name for this method. initializePrototype (just to get you thinking, I don't like that either)

BarController.prototype.dataElementType = Rectangle;
Copy link
Contributor

Choose a reason for hiding this comment

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

I can't find anyone else online suggesting that prototype caused them an issue. Maybe babel is transforming it first which is causing issues? (e.g. Object.defineProperty(BarController.prototype, ... or something). Otherwise I think I'd report an issue to Rollup and see what they say before we introduce this workaround because it's a bit weird to me that this would cause trouble but BarController.id = 'bar'; wouldn't

Copy link
Contributor Author

@sgratzl sgratzl May 23, 2020 •
edited

Choose a reason for hiding this comment

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

take a look at

https://rollupjs.org/repl/

and you can play around yourself. btw. webpack is not better, in this simple test it not even shaked out the .id variant

Copy link
Contributor

@benmccann benmccann May 23, 2020 •
edited

Choose a reason for hiding this comment

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

Should we move id inside preRegister as well then to help webpack?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

then .preRegister still remains which has the same effect.

I don't know how to have something like .registerController(LineController) without having additional attributes like id, defaults, preRegister.

one could extract everything to a separate method: registerLineController but this be not as handy

Copy link
Contributor

Choose a reason for hiding this comment

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

Good point. Thanks for that repl! It's very informative and helpful

I filed an issue with rollup to see if there's anything they can do on their side, but we should figure out what's possible just on our end in the meantime

I don't know how to have something like .registerController(LineController) without having additional attributes like id, defaults, preRegister.

What if we do something like:

export function line() {
  const defaults = { ... };
  registry.registerContoller('line', LineController, defaults);
}

Then users could do:

import { Chart, line, categoryScale, lineScale } from 'chart.js';
[ line, categoryScale, lineScale ].forEach(f => f());


BarController.prototype.dataElementOptions = [
'backgroundColor',
'borderColor',
'borderSkipped',
'borderWidth',
'barPercentage',
'barThickness',
'categoryPercentage',
'maxBarThickness',
'minBarLength'
];
};
34 changes: 20 additions & 14 deletions src/controllers/controller.bubble.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,11 @@
import DatasetController from '../core/core.datasetController';
import defaults from '../core/core.defaults';
import {Point} from '../elements/index';
import {resolve} from '../helpers/helpers.options';

defaults.set('bubble', {
/**
* @type {any}
*/
const defaults = {
animation: {
numbers: {
properties: ['x', 'y', 'borderWidth', 'radius']
Expand All @@ -26,7 +28,7 @@ defaults.set('bubble', {
}
}
}
});
};

export default class BubbleController extends DatasetController {

Expand Down Expand Up @@ -163,14 +165,18 @@ export default class BubbleController extends DatasetController {
}
}

BubbleController.prototype.dataElementType = Point;

BubbleController.prototype.dataElementOptions = [
'backgroundColor',
'borderColor',
'borderWidth',
'hitRadius',
'radius',
'pointStyle',
'rotation'
];
BubbleController.id = 'bubble';
BubbleController.defaults = defaults;
BubbleController.preRegister = () => {
BubbleController.prototype.dataElementType = Point;

BubbleController.prototype.dataElementOptions = [
'backgroundColor',
'borderColor',
'borderWidth',
'hitRadius',
'radius',
'pointStyle',
'rotation'
];
};
34 changes: 20 additions & 14 deletions src/controllers/controller.doughnut.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import DatasetController from '../core/core.datasetController';
import defaults from '../core/core.defaults';
import {Arc} from '../elements/index';
import {isArray, valueOrDefault} from '../helpers/helpers.core';

Expand All @@ -11,7 +10,10 @@ const PI = Math.PI;
const DOUBLE_PI = PI * 2;
const HALF_PI = PI / 2;

defaults.set('doughnut', {
/**
* @type {any}
*/
const defaults = {
animation: {
numbers: {
type: 'number',
Expand Down Expand Up @@ -85,7 +87,7 @@ defaults.set('doughnut', {
}
}
}
});
};

function getRatioAndOffset(rotation, circumference, cutout) {
let ratioX = 1;
Expand Down Expand Up @@ -329,14 +331,18 @@ export default class DoughnutController extends DatasetController {
}
}

DoughnutController.prototype.dataElementType = Arc;

DoughnutController.prototype.dataElementOptions = [
'backgroundColor',
'borderColor',
'borderWidth',
'borderAlign',
'hoverBackgroundColor',
'hoverBorderColor',
'hoverBorderWidth',
];
DoughnutController.id = 'doughnut';
DoughnutController.defaults = defaults;
DoughnutController.preRegister = () => {
DoughnutController.prototype.dataElementType = Arc;

DoughnutController.prototype.dataElementOptions = [
'backgroundColor',
'borderColor',
'borderWidth',
'borderAlign',
'hoverBackgroundColor',
'hoverBorderColor',
'hoverBorderWidth',
];
};
11 changes: 8 additions & 3 deletions src/controllers/controller.horizontalBar.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
import BarController from './controller.bar';
import defaults from '../core/core.defaults';

defaults.set('horizontalBar', {
/**
* @type {any}
*/
const defaults = {
hover: {
mode: 'index',
axis: 'y'
Expand Down Expand Up @@ -36,7 +38,7 @@ defaults.set('horizontalBar', {
mode: 'index',
axis: 'y'
}
});
};

export default class HorizontalBarController extends BarController {

Expand All @@ -54,3 +56,6 @@ export default class HorizontalBarController extends BarController {
return this._cachedMeta.yAxisID;
}
}

HorizontalBarController.id = 'horizontalBar';
HorizontalBarController.defaults = defaults;
72 changes: 39 additions & 33 deletions src/controllers/controller.line.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,13 @@
import DatasetController from '../core/core.datasetController';
import defaults from '../core/core.defaults';
import {Line, Point} from '../elements/index';
import {valueOrDefault} from '../helpers/helpers.core';
import {isNumber} from '../helpers/helpers.math';
import {resolve} from '../helpers/helpers.options';

defaults.set('line', {
/**
* @type {any}
*/
const defaults = {
showLines: true,
spanGaps: false,

Expand All @@ -21,7 +23,7 @@ defaults.set('line', {
type: 'linear',
},
}
});
};

export default class LineController extends DatasetController {

Expand Down Expand Up @@ -157,34 +159,38 @@ export default class LineController extends DatasetController {
}
}

LineController.prototype.datasetElementType = Line;

LineController.prototype.dataElementType = Point;

LineController.prototype.datasetElementOptions = [
'backgroundColor',
'borderCapStyle',
'borderColor',
'borderDash',
'borderDashOffset',
'borderJoinStyle',
'borderWidth',
'capBezierPoints',
'cubicInterpolationMode',
'fill'
];

LineController.prototype.dataElementOptions = {
backgroundColor: 'pointBackgroundColor',
borderColor: 'pointBorderColor',
borderWidth: 'pointBorderWidth',
hitRadius: 'pointHitRadius',
hoverHitRadius: 'pointHitRadius',
hoverBackgroundColor: 'pointHoverBackgroundColor',
hoverBorderColor: 'pointHoverBorderColor',
hoverBorderWidth: 'pointHoverBorderWidth',
hoverRadius: 'pointHoverRadius',
pointStyle: 'pointStyle',
radius: 'pointRadius',
rotation: 'pointRotation'
LineController.id = 'line';
LineController.defaults = defaults;
LineController.preRegister = () => {
LineController.prototype.datasetElementType = Line;
Copy link
Member

Choose a reason for hiding this comment

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

Somehow I don't like a function that modifies the prototype. But I don't really know why :)
Other option that comes to mind is return the things from a method or methods.
getDataElementType etc.


LineController.prototype.dataElementType = Point;

LineController.prototype.datasetElementOptions = [
'backgroundColor',
'borderCapStyle',
'borderColor',
'borderDash',
'borderDashOffset',
'borderJoinStyle',
'borderWidth',
'capBezierPoints',
'cubicInterpolationMode',
'fill'
];

LineController.prototype.dataElementOptions = {
backgroundColor: 'pointBackgroundColor',
borderColor: 'pointBorderColor',
borderWidth: 'pointBorderWidth',
hitRadius: 'pointHitRadius',
hoverHitRadius: 'pointHitRadius',
hoverBackgroundColor: 'pointHoverBackgroundColor',
hoverBorderColor: 'pointHoverBorderColor',
hoverBorderWidth: 'pointHoverBorderWidth',
hoverRadius: 'pointHoverRadius',
pointStyle: 'pointStyle',
radius: 'pointRadius',
rotation: 'pointRotation'
};
};
18 changes: 11 additions & 7 deletions src/controllers/controller.pie.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,11 +1,15 @@
import DoughnutController from './controller.doughnut';
import defaults from '../core/core.defaults';
import {clone} from '../helpers/helpers.core';
import {merge} from '../helpers/helpers.core';

defaults.set('pie', clone(defaults.doughnut));
defaults.set('pie', {
cutoutPercentage: 0
});

// Pie charts are Doughnut chart with different defaults
export default DoughnutController;
export default class PieController extends DoughnutController {

}
PieController.id = 'pie';
PieController.preRegister = () => {
DoughnutController.preRegister();
PieController.defaults = merge({}, [DoughnutController.defaults, {
sgratzl marked this conversation as resolved.
Show resolved Hide resolved
cutoutPercentage: 0
}]);
};
34 changes: 20 additions & 14 deletions src/controllers/controller.polarArea.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,10 +1,12 @@
import DatasetController from '../core/core.datasetController';
import defaults from '../core/core.defaults';
import {Arc} from '../elements/index';
import {toRadians} from '../helpers/helpers.math';
import {resolve} from '../helpers/helpers.options';

defaults.set('polarArea', {
/**
* @type {any}
*/
const defaults = {
animation: {
numbers: {
type: 'number',
Expand Down Expand Up @@ -72,7 +74,7 @@ defaults.set('polarArea', {
}
}
}
});
};

function getStartAngleRadians(deg) {
// radialLinear scale draws angleLines using startAngle. 0 is expected to be at top.
Expand Down Expand Up @@ -221,14 +223,18 @@ export default class PolarAreaController extends DatasetController {
}
}

PolarAreaController.prototype.dataElementType = Arc;

PolarAreaController.prototype.dataElementOptions = [
'backgroundColor',
'borderColor',
'borderWidth',
'borderAlign',
'hoverBackgroundColor',
'hoverBorderColor',
'hoverBorderWidth'
];
PolarAreaController.id = 'polarArea';
PolarAreaController.defaults = defaults;
PolarAreaController.preRegister = () => {
PolarAreaController.prototype.dataElementType = Arc;

PolarAreaController.prototype.dataElementOptions = [
'backgroundColor',
'borderColor',
'borderWidth',
'borderAlign',
'hoverBackgroundColor',
'hoverBorderColor',
'hoverBorderWidth'
];
};