-
Notifications
You must be signed in to change notification settings - Fork 58
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
UIP-2153: Transition warning sometimes fires when it shouldn't #58
UIP-2153: Transition warning sometimes fires when it shouldn't #58
Conversation
RavenNumber of Findings: 0 |
Codecov Report
@@ Coverage Diff @@
## master #58 +/- ##
==========================================
+ Coverage 97.72% 97.73% +0.02%
==========================================
Files 28 28
Lines 1356 1363 +7
==========================================
+ Hits 1325 1332 +7
Misses 31 31 Continue to review full report at Codecov.
|
complete(); | ||
}); | ||
|
||
_endTransitionSubscription = getTransitionDomNode()?.onTransitionEnd?.skip(skipCount)?.take(1)?.listen((_) { | ||
timer.cancel(); | ||
|
||
if (_transitionEndTimer != null) { |
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.
There should be a private helper function to do this. Also you can just use a null-aware operator.
void _cancelTransitionEndTimer() {
_transitionEndTimer?.cancel();
_transitionEndTimer = null;
}
+1 |
I'm seeing warnings when I shouldn't when double-clicking the datepicker button rapidly when it's already open. 😢 |
@@ -187,16 +190,19 @@ abstract class AbstractTransitionComponent<T extends AbstractTransitionProps, S | |||
skipCount = 0; | |||
} | |||
|
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.
Hm, I wonder if we should just always do the following here:
_cancelTransitionEventListener();
_cancelTransitionEndTimer();
to clean up any previous calls
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.
@greglittlefield-wf Do you think the extraneous warnings still appearing could be resolved by placing _cancelTransitionEndTimer()
at the top of the handlePreShowing
method?
I tried it and it seems to work, but I'm not sure if doing this defeats the purpose of the timer in the first place.
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.
Potentially, but wouldn't that be accomplishing the same as what I linked above, albeit in a subset of cases?
+10 |
+1 |
QA +1
Merging. |
…to-dom-builders Warn when custom props are forwarded to a dom component
Ultimate problem:
The newly-added transition warning (UIP-1802),
VALIDATION WARNING: The number of transitions expected to complete have not completed. Something is most likely wrong.
fires sometimes when it shouldn't.For instance, sometimes
transitionend
events never fire on purpose, such as when the user cancels the transition (think hiding a popover while it's still showing). This results in a warning when it should not.I also noticed in the code that the timer does not unsubscribe the listener function, which it probably should.
How it was fixed:
_transitionEndTimer
, that keeps track of whether a transition timeout has occurred._transitionEndTimer
is canceled as well.Testing suggestions:
Potential areas of regression: