Create common logic for resolving element options #6004
Conversation
benmccann
force-pushed
the
element-resolve
branch
from
60e6341
to
a1a7e76
Compare
| @@ -112,9 +116,22 @@ module.exports = DatasetController.extend({ | |||
| var value = dataset.data[index]; | |||
| var yScale = me.getScaleForId(meta.yAxisID); | |||
| var xScale = me.getScaleForId(meta.xAxisID); | |||
| var POINT_ELEMENT_OPTIONS = { | |||
There was a problem hiding this comment.
Could these live in element.point instead, or are there conflicts in naming? Not sure they should, just thinking out loud.
There was a problem hiding this comment.
I'm not sure I understand what the suggestion is.
To make sure we're on the same page, this is a mapping from element option name to dataset option name
There was a problem hiding this comment.
I think it should stay in this file as long as we don't need it elsewhere
There was a problem hiding this comment.
I was asking if those option names are always the same, and could therefore be defined only once in the place they are effectively used in (elements.point.js).
Quick check reveals that this is true for line and radar, but bubble does not have the point prefix.
So the answer is no (or not yet).
Ideally this map would come from the defaults for the element (and prefix would be elements name or the options would be scoped (point.borderColor insteand of pointBorderColor)). Maybe in v3.
| borderJoinStyle: 'borderJoinStyle', | ||
| fill: 'fill', | ||
| cubicInterpolationMode: 'cubicInterpolationMode' | ||
| }; |
There was a problem hiding this comment.
We should not use a map for the line options but instead, tension should be a special case (independently resolved as here), else we will evaluate the same value twice for all properties.
IMO, the point prefixed options logic is a special case which shouldn't be use as common implementation when resolving element options.
| @@ -112,9 +116,22 @@ module.exports = DatasetController.extend({ | |||
| var value = dataset.data[index]; | |||
| var yScale = me.getScaleForId(meta.yAxisID); | |||
| var xScale = me.getScaleForId(meta.xAxisID); | |||
| var POINT_ELEMENT_OPTIONS = { | |||
There was a problem hiding this comment.
I think it should stay in this file as long as we don't need it elsewhere
| @@ -148,46 +165,33 @@ module.exports = DatasetController.extend({ | |||
| /** | |||
| * @private | |||
| */ | |||
| _resolveElementOptions: function(point, index) { | |||
| _resolveElementOptions: function(element, elementName, optionNameMap, index, scriptable) { | |||
There was a problem hiding this comment.
I would not try to share this method between line and point elements (at least not yet) but instead duplicate it into _resolveLineOptions (and rename this one to _resolvePointOptions).
| dataset[key], | ||
| options[key] | ||
| elementOpts[key] | ||
| ], context, index); |
There was a problem hiding this comment.
We should avoid the extra dataset[optionNameMap[key]] which is always the same as dataset[key] for line (except for tension). Duplicating this method would be the easier way to do that (see previous comment).
|
Created a new PR with the suggested changes since it was easier to start from master #6005 |
No description provided.