[deps] upgrade visx-xychart to react 17 #1
Conversation
| describe('<BaseAxis />', () => { | ||
| it('should be defined', () => { | ||
| expect(BaseAxis).toBeDefined(); | ||
| }); | ||
| it('should use scale=xScale for orientation=top/bottom', () => { |
There was a problem hiding this comment.
hi @williaster, this is tricky to do within RTL because it is checking on function xScale being passed down through props and this counts as a checking on implementation detail. After consideration, I am thinking of removing this test for now... Do you think this test is necessary?
There was a problem hiding this comment.
yeah I think that's a totally fair critique. I don't think it's essential to have the tests, tho I wonder if there's a more RTL / functional type of test. My only thought is to create actual scales say
const xScale = scaleLinear({ domain: [0, 1, 2], range: [0, 10] });
const yScale = scaleLinear({ domain: ['a', 'b', 'c'], range: [0, 10] });and then look for tick values that match those. that's a decent amount of work to do tho and don't want to add more and more to this migration so I'm fine skipping it!
There was a problem hiding this comment.
ill give that a shot.
I did const xScale ={ domain: jest.fn(()=>[0, 1, 2]), range: jest.fn(() => [0, 10]) };
it complains about missing tickPosition function, i don't know where that one is being passed from
There was a problem hiding this comment.
That actually works! please check my changes at L56
🚀
packages/visx-xychart/package.json
Outdated
| @@ -36,7 +36,7 @@ | |||
| "access": "public" | |||
| }, | |||
| "peerDependencies": { | |||
| "react": "^16.4.0-0", | |||
| "react": "^17.0.0", | |||
There was a problem hiding this comment.
should this be ^16.4.0 || ^17.0.0-0 or should we go all in on 17?
There was a problem hiding this comment.
i'd say 17.0.0 because visx-xychart uses react-spring@v9 correct?
There was a problem hiding this comment.
ah good point. looks like that supports ^16.8.0 || ^17.0.0 so maybe we should match that? then people can still use xychart without updating to 17.
| describe('<BaseAxis />', () => { | ||
| it('should be defined', () => { | ||
| expect(BaseAxis).toBeDefined(); | ||
| }); | ||
| it('should use scale=xScale for orientation=top/bottom', () => { |
There was a problem hiding this comment.
yeah I think that's a totally fair critique. I don't think it's essential to have the tests, tho I wonder if there's a more RTL / functional type of test. My only thought is to create actual scales say
const xScale = scaleLinear({ domain: [0, 1, 2], range: [0, 10] });
const yScale = scaleLinear({ domain: ['a', 'b', 'c'], range: [0, 10] });and then look for tick values that match those. that's a decent amount of work to do tho and don't want to add more and more to this migration so I'm fine skipping it!
| expect(rects.at(1).prop('fill')).not.toBe('banana'); | ||
| expect(rects.at(2).prop('fill')).toBe('banana'); | ||
| expect(rects.at(3).prop('fill')).not.toBe('banana'); | ||
| const rects = container.querySelectorAll('rect'); |
There was a problem hiding this comment.
glad some of these were more straight forward to refactor 😅
| expect(circles.at(0).prop('fill')).toBe('banana'); | ||
| expect(circles.at(1).prop('fill')).not.toBe('banana'); | ||
| const circles = container.querySelectorAll('circle'); | ||
| expect(circles[0]).toHaveAttribute('fill', 'banana'); |
There was a problem hiding this comment.
so much more DOM/vanilla javascript like versus enzyme 💯
| expect(wrapper.find(TooltipProvider)).toHaveLength(1); | ||
| // the XYChart should auto-resize to it's parent size | ||
| const Wrapper = getByTestId('wrapper'); | ||
| expect(Wrapper.firstChild).toHaveStyle('width: 100%; height: 100%'); |
| <rect /> | ||
| </XYChart>, | ||
| ); | ||
| expect(wrapper.find(DataProvider)).toHaveLength(1); |
There was a problem hiding this comment.
not sure how best to test this one since context is an implementation detail kind of by definition.
There was a problem hiding this comment.
yea, because when this component gets rendered, you don't really see a DataProvider, EventEmitterProvider or TooltipProvider in your DOM anyways.
Overview
find(Component)pattern in enzyme to testing-library: