-
Notifications
You must be signed in to change notification settings - Fork 380
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
chore(react-chart): refactor series #1279
chore(react-chart): refactor series #1279
Conversation
Add subheader 'react-chart' to the PR title, please |
valueField="ru" | ||
argumentField="year" | ||
/> | ||
<AreaSeries | ||
color="black" |
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.
Do we want to see this colors in this demo?
}) => ({ | ||
d: arc() | ||
.innerRadius(0) | ||
.outerRadius(radius) |
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.
Why the 'innerRadius' and 'outerRadius' options do not apply?
const dPoint = symbol().size([size ** 2]).type(symbolCircle)(); | ||
const offSet = scales.xScale.bandwidth ? scales.xScale.bandwidth() / 2 : 0; | ||
export const pointAttributes = () => { | ||
const dPoint = symbol().size([DEFAULT_POINT_SIZE ** 2]).type(symbolCircle)(); |
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.
In the doc of the series the 'point.size' option remains. And it looks like BC, that should be described.
@@ -9,137 +9,146 @@ import { | |||
} from 'd3-shape'; | |||
import { createScale } from '../../utils/scale'; | |||
|
|||
const getX = ({ x }) => x; | |||
const getX = ({ x, width }) => x + (width / 2); |
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.
For that new code no tests
export const findSeriesByName = (name, series) => series.find( | ||
seriesItem => seriesItem.uniqueName === name, | ||
); | ||
export const barCoordinates = ( |
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 are no tests on this method. It is the same with the dArea, dLine, dBar and dSpline methods.
@@ -5,12 +5,11 @@ import classNames from 'classnames'; | |||
export class Area extends React.PureComponent { | |||
render() { | |||
const { | |||
x, y, className, pointComponent, pointStyle, coordinates, path, themeColor, ...restProps | |||
className, pointComponent, pointStyle, coordinates, path, color, ...restProps |
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.
The 'pointComponent' property is not needed in the area, line, spline series
const { coordinates } = props; | ||
return ( | ||
<React.Fragment> | ||
<LineSeries.Path {...props} coordinates={coordinates.slice(0)} /> |
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.
Looks like there are extra actions with the coordinates
property.
@@ -8,7 +8,6 @@ export class AreaSeries extends React.PureComponent { | |||
return ( | |||
<AreaSeriesBase | |||
seriesComponent={Area} | |||
pointComponent={() => null} | |||
{...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.
The string AreaSeries.Point = Point
is not expected there. The same in the material-ui theme in all line series (spline, line, area)
BREAKING CHANGES:
Previously, we had the
pointComponent
property for drawing and customizingLineSeries
,SplineSeries
, andAreaSeries
plugin points. But since we have a scatter series, this property has become unnecessary. Use theScatterSeries
plugin or customize thepathComponent
of the corresponding plugin.fixes #1271