[Tabs] Forward refs #14714
[Tabs] Forward refs #14714
Conversation
eps1lon
added
breaking change
component: tabs
| it('should work server-side', () => { | ||
| const wrapper2 = shallow( | ||
| /* wrapping Tabs in forwardRef calls componentDidMount :( | ||
| it('should not have style server-side', () => { |
There was a problem hiding this comment.
@oliviertassinari What issue is this trying to solve? I see that we currently move the indicator to the Tab on the first render but move it into the own render tree after it's mounted. I don't understand what this achieves. Could you elaborate what behavior we're expecting to see? Maybe I can come up with a better test for this behavior then.
There was a problem hiding this comment.
I believe that we should be using the server-side rendering API of enzyme, shallow was a poor fit for this test case. It's solving 3 problems, the root problem comes from the need of the browser layout API to position the indicator:
- We want to display the indicator at the right location when doing the server-side rendering. We shouldn't have to wait for the client side reconciliation.
- We don't want to see the indicator to disappear when switching between different pages client-side.
- We want the indicator to be correctly positioned when the font is loaded and changes the tab width.
There was a problem hiding this comment.
I couldn't find any. What's the ssr rendering API of enzyme?
There was a problem hiding this comment.
I had https://airbnb.io/enzyme/docs/api/render.html in mind.
There was a problem hiding this comment.
Will look into it. Doesn't make any statements about lifecycles so not sure this helps.
| .at(0) | ||
| .simulate('resize'); | ||
|
|
||
| window.dispatchEvent(new window.Event('resize', {})); |
There was a problem hiding this comment.
Decouples test from specific implementation. We don't actually care how we listen to the resize event. Just that we listen to window.onResize. This should still pass if we switch to a useEffect and window.addEventListener.
Details of bundle changes.Comparing: 23549d4...5ebc9db
|
eps1lon
force-pushed
the
feat/Tabs/forwardRef
branch
from
a3ba0a5
to
e98f42e
Compare
Testing my own ability to write meaningful tests here. Previously I converted to forwardRef and then fixed tests. Let's see if we actually write better tests or still do the same mistake. Better tests would result in minimal breakage upon adding forwardRef.
eps1lon
force-pushed
the
feat/Tabs/forwardRef
branch
from
e98f42e
to
84ae0f4
Compare
oliviertassinari
force-pushed
the
feat/Tabs/forwardRef
branch
from
a1511a6
to
5ebc9db
Compare
Little bit different approach this time: Refactored tests first then implementation. Definitely still biased because I knew how the implementation would look like but still increased confidence in tests for me.
I need to investigate the shallow + disableLifeCycleIssue. This is causing enzyme to call componentDidMount if we wrap it in
forwardRef.Continues #14536