-
Notifications
You must be signed in to change notification settings - Fork 11.9k
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.
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
Create common logic for resolving element options #6004
Conversation
60e6341
to
a1a7e76
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure if its still early for this refactor, but it seems logical to me.
@@ -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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should stay in this file as long as we don't need it elsewhere
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure this refactoring is beneficial (or will be) compared to simply duplicate the resolveElementOptions
method and handle line and point separately. We don't need mapping for line and the extra code / logic / lookup may not justify a refactor.
borderJoinStyle: 'borderJoinStyle', | ||
fill: 'fill', | ||
cubicInterpolationMode: 'cubicInterpolationMode' | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.