-
Notifications
You must be signed in to change notification settings - Fork 835
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
Better PropType validation for data in AbstractSeries; Tests for using Accessors #765
Conversation
@@ -39,7 +39,10 @@ const propTypes = { | |||
...getScalePropTypesByAttribute('color'), | |||
width: PropTypes.number, | |||
height: PropTypes.number, | |||
data: PropTypes.arrayOf(PropTypes.object), | |||
data: PropTypes.arrayOf(PropTypes.oneOfType([ |
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.
add some documentation/testing to show that this case is supported?
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.
you mean testing that the warning is not triggered anymore? I did test this manually, but I am not sure how to achieve this through automated tests.
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.
more like add a test case demonstrating that this is a valid way to input data (I don't remember writing any but I might have?) and add a note in the documentation showing that this is a supported case (again I may have already done that but i don't think i did??). If it's the case that I am being dumb and these things already exist than this gets a stamp
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 great! Sorry for adding a bunch of layers to a tiny type checking pr, but due diligence etc is important. Stamp Stamp Stamp StampStampStampStampStampStampStampStampStampStampStampStampStamp
width={300} | ||
height={300} | ||
getX={d => d[0]} | ||
getY={d => d[1]> |
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 know that this PR isn't implementing accessors, but now I'm curious–do we also support accessors on series components? That seems like a more common use case that people might have (and if we support it we should document it).
const data = [
{fruit: 'apple', bought: 23, sold: 18},
{fruit: 'banana', bought: 28, sold: 27}
];
<XYPlot
...
data={data}
getX{d => d.fruit}
/>
<LineSeries getY={d => d.bought} />
<LineSeries getY={d => d.sold} />
</XYPlot>
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.
Series level accessors might be supported, though if they are the functionality is untested. The only guaranteed support that we offer is on a plot level. I'd support adding accessors on series, seems like a reasonable idea.
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 don't think they are supported. the accessors seem to be passed down from XYPlot
.
…g Accessors (uber#765) * Make the Abstract Series data PropType validation accomodate accessors * Add tests for accessors; update documentation
…g Accessors (uber#765) * Make the Abstract Series data PropType validation accomodate accessors * Add tests for accessors; update documentation
Fixes #724. This change will prevent prop-validation for throwing a warning when users use
data
accessors (for using arrays, for example).