add mountOnEnter #144
add mountOnEnter #144
Conversation
jquense
force-pushed
the
mountOnEnter
branch
from
21797b7
to
22ee7f6
Compare
|
🏏 |
src/Transition.js
Outdated
|
|
||
| if (nextProps.in && this.state.status === UNMOUNTED) { | ||
| // Start enter transition in componentDidUpdate. | ||
| this.setState({ status: EXITED }); | ||
| } | ||
| } | ||
|
|
||
| componentDidUpdate() { | ||
| const status = this.state.status; |
There was a problem hiding this comment.
surprised we don't destructure here
src/Transition.js
Outdated
| } | ||
| // EXITED is always a transitional state to either ENTERING or UNMOUNTED | ||
| // when using unmountOnExit. | ||
| const needsTransitionalUpdate = ( |
There was a problem hiding this comment.
this is a little confusing because a transitional update is needed when mountOnEnter and in are both set
src/Transition.js
Outdated
|
|
||
| if (nextProps.in && this.state.status === UNMOUNTED) { | ||
| // Start enter transition in componentDidUpdate. | ||
| this.setState({ status: EXITED }); |
There was a problem hiding this comment.
just for clarity i liked not setting _propsUpdated on this path, since the transitional update logic will handle this
src/Transition.js
Outdated
| } | ||
| } | ||
| else if (needsTransitionalUpdate) { |
There was a problem hiding this comment.
ahh... maybe this would be better named needsUnmountForExit or something?
There was a problem hiding this comment.
or maybe needsTransitionalUpdate && unmountOnExit
|
Thinking about this a bit more – can we just have a I think I was overly perf focused when I wrote this initially, and that doing so was the wrong call. |
|
Some of the reason this is in CDU is because it needs to respond to transitional changes that result from I'll be the first to admit i'm not super happy with the awkward logic branches here. |
src/Transition.js
Outdated
| } | ||
|
|
||
| componentWillReceiveProps(nextProps) { | ||
| this._propsUpdated = true |
There was a problem hiding this comment.
wait why do we always defer rendering ENTERING until the next render cycle?
There was a problem hiding this comment.
oh i see, there's a test asserting so
|
@jquense wdyt? |
|
Looks good to me, from what I could tell. I haven't had a moment to try it out tho. IF you feel good about it |
tho
|
carpe yolo |
Solves the dang lazy tab loading thing downstream, at least for animation.
also cleans up the logic a bit (I think please double check)