-
Notifications
You must be signed in to change notification settings - Fork 4
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
Upgrade Recharts. (PP-1531) #127
Conversation
// These values are needed for testing, in some cases. | ||
export const COLLECTION_BAR_CHART_TEST_FLAG_KEY = | ||
"COLLECTION_BAR_CHART_TEST_FLAG_KEY"; | ||
const TestResponsiveContainer = ({ children }) => ( | ||
<RechartsResponsiveContainer width={800} height={800}> | ||
{children} | ||
</RechartsResponsiveContainer> | ||
); |
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 added this and the overall testingFlags
approach to:
- provide a higher-level way to support some tricky test scenarios without having to drill a prop through a bunch of components; and
- to keep as much of the mocking near the component, so that it is easier to keep them aligned.
Would appreciate feedback on this. Bad idea? Any better ideas?
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 general I'd rather have control of the tests be completely inside the test files, rather than interleaved into the components that are tested. If I'm reading this right, this test flag is overriding the width
and height
props of RechartsResponsiveContainer
. My preference would be instead of setting a testing flag, allow specifying the width and height overrides directly. So your context might contain something like
config: { // these key names are off the cuff, I don't care much what they are exactly
StatsCollectionsBarChart: {
width: 800,
height: 800
}
}
That way it's easy to see in the test what exactly is happening, and different tests can pass different settings if they want to, instead of having them hardcoded into the component. From the component's POV, it doesn't know or care if it's under test, it just has a feature (that can itself be tested) that some settings are configurable from context.
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.
Thanks, @ray-lee! That's very helpful. I think it makes a lot of sense and reduces complexity. I've made changes in that direction.
6a2b49e
to
47d8419
Compare
@ray-lee This has been rebased onto |
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 good!
Description
Recharts
package to a modern version.jest
.Note: This work builds on #126 and should be merged after it.
Motivation and Context
[Jira PP-1531]
How Has This Been Tested?
Checklist: