refactor(Transition): improve timeout prop as a object and make optional
#629
Conversation
|
Marked as draft because have some questions that appearered after looking at method |
|
Ok, the purpose of this PR deviates from original scope, I'll rename, and adapt to new changes. |
iamandrewluca
changed the title
getTimeouts to a pure functiontimeout prop as a object and make optional
| setTimeout(this.nextCallback, 0) | ||
| return | ||
| } | ||
| if (node && this.props.addEndListener && timeout == null) { |
There was a problem hiding this comment.
@taion why node check is needed here in onTransitionEnd?
addEndListener check is not enough for this?
Or is here for some historic reasons?
Tests pass if node check is removed from if check.
There was a problem hiding this comment.
i think this is a legacy thing from the conversion to use nodeRef; if there's no associated node, then previously it was impossible to call addEndListener, because the first arg was expected to be the node.
There was a problem hiding this comment.
Is there any case when node may be missing?
There was a problem hiding this comment.
might be possible if e.g. the transition is rendering a child that's actually not there, maybe?
| setTimeout(this.nextCallback, 0) | ||
| return | ||
| } | ||
| if (node && this.props.addEndListener && timeout == null) { |
There was a problem hiding this comment.
i think this is a legacy thing from the conversion to use nodeRef; if there's no associated node, then previously it was impossible to call addEndListener, because the first arg was expected to be the node.
|
|
||
| if (timeout != null) { | ||
| setTimeout(this.nextCallback, timeout) | ||
| this.props.addEndListener(maybeNode, maybeNextCallback) |
There was a problem hiding this comment.
this is not equivalent to the original code. previously, if:
nodewas definedaddEndListenerwas definedtimeoutwas defined
then we would call addEndListener and we would call setTimeout with this.nextCallback.
in this revision, there's no way for both addEndListener and setTimeout to get called
There was a problem hiding this comment.
Is this the right behaviour for both of them to call nextCallback?
Does setNextCallback appeared in codebase because of this, to call callback only once?
There was a problem hiding this comment.
i think the idea is that, if there's a timeout and an end listener, that the first one that triggers will "win"
…ional Purpose of #464 was to make timeout optional and default to 0 if `addEndListener` prop is missing, but propType was still marked as required Also removed unused class `SimpleSet`
Purpose of #464 was to make timeout optional and default to 0
if
addEndListenerprop is missing, but propType was still marked asrequired
Also removed unused class
SimpleSet