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

Introduce scriptable options (bubble chart) #4671

Merged
merged 3 commits into from Aug 24, 2017
Merged

Introduce scriptable options (bubble chart) #4671

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
b59d81f
Introduce scriptable options (bubble chart)
simonbrunel Aug 16, 2017
d1be09f
Move test event helper under jasmine namespace
simonbrunel Aug 18, 2017
a456414
Minimal set of context properties
simonbrunel Aug 19, 2017
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
9 changes: 3 additions & 6 deletions docs/general/options.md
View file
Open in desktop
Expand Up @@ -37,12 +37,9 @@ The option context is used to give contextual information when resolving options

The context object contains the following properties:

- `datasetIndex`: index of the current dataset
- `datasetCount`: number of datasets
- `dataset`: dataset at index `datasetIndex`
- `dataIndex`: index of the current data
- `dataCount`: number of data
- `data`: value of the current data
- `chart`: the associated chart
Copy link
Member Author

Choose a reason for hiding this comment

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

Since this is part of the public API, we need to be sure we want to expose all these properties.

Thoughts?

Copy link
Member

Choose a reason for hiding this comment

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

I'm OK with these properties

Copy link
Member Author

Choose a reason for hiding this comment

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

At least, we need to expose chart, datasetIndex and dataIndex properties.

The other values can be retrieved as follow:

  • datasetCount: chart.data.datasets.length
  • dataset: chart.data.datasets[datasetIndex]
  • dataCount: chart.data.datasets[datasetIndex].data.length
  • data: chart.data.datasets[datasetIndex].data[dataIndex]

dataset is always needed to resolve options (since it contains some of them), so it would not cost more to include it to the context. I think most use cases of scriptable options will be to derive the data value, however data is (almost) never required to resolve options, so it's an additional cost to expose it to the context. I'm really not sure about exposing (right now) datasetCount and dataCount.

If we expose only chart, datasetIndex, dataIndex and dataset:

  • datasetCount: chart.data.datasets.length
  • dataCount: dataset.data.length
  • data: dataset.data[dataIndex]

Copy link
Member

Choose a reason for hiding this comment

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

I think chart, datasetIndex, dataIndex and dataset is the minimum. The only reason to include the other properties is to save users code if they want to get those values.

I'm also OK with the minimal set of properties to keep our code smaller.

Copy link
Member Author

Choose a reason for hiding this comment

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

We can start with the minimal set and add more if requested. Like that we limit probabilities of later deprecations.

- `dataIndex`: index of the current data
- `dataset`: dataset at index `datasetIndex`
- `datasetIndex`: index of the current dataset

**Important**: since the context can represent different types of entities (dataset, data, etc.), some properties may be `undefined` so be sure to test any context property before using it.
15 changes: 8 additions & 7 deletions samples/scriptable/bubble.html
View file
Open in desktop
Expand Up @@ -30,13 +30,13 @@
utils.srand(110);

function colorize(opaque, context) {
var data = context.data;
var x = data.x / 100;
var y = data.y / 100;
var value = context.dataset.data[context.dataIndex];
var x = value.x / 100;
var y = value.y / 100;
var r = x < 0 && y < 0 ? 250 : x < 0 ? 150 : y < 0 ? 50 : 0;
var g = x < 0 && y < 0 ? 0 : x < 0 ? 50 : y < 0 ? 150 : 250;
var b = x < 0 && y < 0 ? 0 : x > 0 && y > 0 ? 250 : 150;
var a = opaque ? 1 : 0.5 * data.v / 1000;
var a = opaque ? 1 : 0.5 * value.v / 1000;

return 'rgba(' + r + ',' + g + ',' + b + ',' + a + ')';
}
Expand Down Expand Up @@ -86,13 +86,14 @@
},

hoverBorderWidth: function(context) {
return Math.round(8 * context.data.v / 1000);
var value = context.dataset.data[context.dataIndex];
return Math.round(8 * value.v / 1000);
},

radius: function(context) {
var data = context.data;
var value = context.dataset.data[context.dataIndex];
Copy link
Member Author

Choose a reason for hiding this comment

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

@etimberg this is the way how to read the current value with the minimal context. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Looks good 👍

var size = context.chart.width;
var base = Math.abs(data.v) / 1000;
var base = Math.abs(value.v) / 1000;
return (size / 24) * base;
}
}
Expand Down
7 changes: 2 additions & 5 deletions src/controllers/controller.bubble.js
View file
Open in desktop
Expand Up @@ -140,12 +140,9 @@ module.exports = function(Chart) {
// Scriptable options
var context = {
chart: chart,
datasetIndex: me.index,
datasetCount: datasets.length,
dataset: dataset,
dataIndex: index,
dataCount: dataset.data.length,
data: data
dataset: dataset,
datasetIndex: me.index
};

var keys = [
Expand Down