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
refactor(react-chart): move point coordinates calculations specifics down to the pointComponent #1753
Conversation
scaleName, | ||
seriesComponent, | ||
pointComponent, | ||
color, |
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.
There is no difference between color
and restProps
here.
pointComponent, | ||
color, | ||
...restProps | ||
} = this.props; | ||
const symbolName = Symbol(name); | ||
const seriesItem = { | ||
getPointTransformer, | ||
createHitTester, | ||
...this.props, |
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.
seriesItem
contains common series configuration - specific and appearance settings are merged later in addSeries
.
common configuration - settings separated from restProps
several lines above.
// It is used to generate unique series dependent attribute names for patterns. | ||
// *symbolName* cannot be used as it cannot be part of DOM attribute name. | ||
const index = series.length; | ||
return [...series, { | ||
...props, | ||
name: getUniqueName(series, props.name), | ||
index, | ||
points: createPoints(props.argumentField, props.valueField, data), | ||
points: createPoints(props.argumentField, props.valueField, data, restProps), | ||
palette, // TODO: For Pie only. Find a better place for it. | ||
color: props.color || palette[index % palette.length], |
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.
color
should be merged to restProps
- otherwise there will be no color
property in a series point.
restProps
should be enumerated here instead of color
.
Actually there is color
property in a series point. But it is so because of {...restProps}
in point-collection.jsx.
Let's add note in point-collection.jsx saying that restProps
are used because of getAnimatedStyle
and scales
.
y: valueScale(point.value), | ||
y1, | ||
width, | ||
spacingForBar: getWidth(argumentScale), |
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 verify this name.
BREAKING CHANGES:
Previously,
pointComponent
ofBarSeries
andPieSeries
accepted precalculated fields (width
andd
, respectively). Now,pointComponent
accepts fields that provide raw data for calculation. This makespointComponent
more flexible as thewidth
andd
fields can now be calculated the way you need.The following substitutions took place:
width
=>barWidth
andmaxBarWidth
d
=>innerRadius
,outerRadius
,maxRadius
,startAngle
, andendAngle