UIP-1802 CSS Transition Timeout/Warning by kaanaksoy-wk · Pull Request #55 · Workiva/over_react

kaanaksoy-wk · 2017-03-09T17:38:20Z

Ultimate problem:

When creating components that utilize the AbstractTransitionComponent, it's possible to set transition to a value that something like an OverlayTrigger doesn't know about, which causes the cleanup of the component to hang.

How it was fixed:

Add a timeout to ensure that the component is cleaned up even if something goes wrong with the transition, and warn the consumer.
- Add a transitionTimeout getter to AbstractTransitionComponent that can be overridden to adjust the desired timeout duration.
- Use a Timer and display a warning after a timeout has occurred.
- Write a test to verify that the timeout actually occurs.

Testing suggestions:

Verify that the tests pass.

Potential areas of regression:

N/A

FYA: @greglittlefield-wf @aaronlademann-wf @jacehensley-wf @clairesarsam-wf @joelleibow-wf

UIP-2052 Release over_react 1.7.0

aviary-wf · 2017-03-09T17:38:24Z

Raven

Number of Findings: 0

codecov-io · 2017-03-09T17:45:49Z

Codecov Report

Merging #55 into release_1.7.0 will increase coverage by 0.01%.
The diff coverage is 100%.

@@                Coverage Diff                @@
##           release_1.7.0      #55      +/-   ##
=================================================
+ Coverage          97.71%   97.72%   +0.01%     
=================================================
  Files                 28       28              
  Lines               1352     1356       +4     
=================================================
+ Hits                1321     1325       +4     
  Misses                31       31

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 425dbaa...f82f71b. Read the comment docs.

jacehensley-wf

Just a few pieces of feedback

jacehensley-wf · 2017-03-10T20:59:17Z

lib/src/component/abstract_transition.dart

  bool get hasTransition => true;

+  /// The duration that can elapse before a transition timeout occurs.
+  Duration get transitionTimeout => const Duration(seconds: 15);


We should have a discussion around the default value for this.

@aaronlademann-wf @greglittlefield-wf @clairesarsam-wf @joelleibow-wf

I'd say make this a prop so that it's configurable, and default it to something much smaller, like 1s, so that consumers see the warning right away.

I feel like a getter is better for this, similar to hasTransition. That way consumers have the ability to either proxy it with a prop or have a custom implementation.

1s sounds good to me tho.

Yeah, the only reason I said prop is that it would be easier to override. Good point, I'm fine leaving it as a getter.

jacehensley-wf · 2017-03-10T20:59:43Z

lib/src/component/abstract_transition.dart

    }

+    var timer = new Timer(transitionTimeout, () {
+      assert(ValidationUtil.warn("A transition timeout has occurred."));


#nit Should use single quotes ' vs "

jacehensley-wf · 2017-03-10T21:02:12Z

lib/src/component/abstract_transition.dart

    }

+    var timer = new Timer(transitionTimeout, () {
+      assert(ValidationUtil.warn("A transition timeout has occurred."));


This should be a more informative message. Something like: 'The amount of transitions expected to complete have not completed. Something is most likely wrong.' The wording isn't great but something that get across the point that we are expecting some amount of transitions and they have not fired.

jacehensley-wf · 2017-03-10T21:06:06Z

test/over_react/component/abstract_transition_test.dart

+
+      TransitionerComponent transitioner = getDartComponent(renderedInstance);
+
+      transitioner.hide();


You should add a expect before and after this that verifies that the state.transtionPhase is TransitionPhase.SHOWN.

jacehensley-wf · 2017-03-13T20:04:54Z

lib/src/component/abstract_transition.dart

+    var timer = new Timer(transitionTimeout, () {
+      assert(ValidationUtil.warn(
+        'The number of transitions expected to complete have not completed. Something is most likely wrong.'
+      ));


Something else that I was thinking about. Should we still call complete if the timer completes? I could go either way, was just wondering what the desired behavior is.

Mm, yes, we should, in order to account for when the warning isn't caught by the time this is deployed

jacehensley-wf · 2017-03-13T21:07:03Z

test/over_react/component/abstract_transition_test.dart

    ..addProps(super.getDefaultProps())
    ..hasTransition = true
    ..initiallyShown = true
+    ..transitionTimeout = const Duration(seconds: 15)


This should be updated to match the default in AbstractTransitionComponent

jacehensley-wf · 2017-03-15T16:21:01Z

+1

aaronlademann-wf · 2017-03-16T15:17:01Z

Merging.+10

teresarevious-wf · 2017-03-16T23:02:55Z

RM +1 verified dependencies

…assist Remove prop navigation assist

leviwith-wf and others added 2 commits March 3, 2017 19:16

Merge pull request Workiva#53 from Workiva/release_1.7.0

cdb035f

UIP-2052 Release over_react 1.7.0

Add timeout to AbstractTransition and write tests

0cc1332

aviary-wf added Security Review Required InfoSec Review Required labels Mar 9, 2017

kaanaksoy-wk changed the base branch from 1.3.0 to release_1.7.0 March 9, 2017 17:44

jacehensley-wf suggested changes Mar 10, 2017

View reviewed changes

Finish making Jace’s recommendations

2af89dc

aviary-wf removed Security Review Required InfoSec Review Required labels Mar 13, 2017

jacehensley-wf reviewed Mar 13, 2017

View reviewed changes

jacehensley-wf added Type: enhancement State: unaddressed feedback labels Mar 14, 2017

kaanaksoy-wk added 2 commits March 14, 2017 16:01

Finish additional recommendations

0d00fd0

Update test to account for timer calling complete

f82f71b

jacehensley-wf approved these changes Mar 15, 2017

View reviewed changes

jacehensley-wf added State: needs review and removed State: unaddressed feedback labels Mar 15, 2017

aaronlademann-wf changed the title ~~UIP-1802 Modal Transition Timeout/Warning~~ UIP-1802 CSS Transition Timeout/Warning Mar 16, 2017

aaronlademann-wf merged commit dcf4727 into Workiva:release_1.7.0 Mar 16, 2017

aaronlademann-wf mentioned this pull request Mar 16, 2017

UIP-2091 Release over_react 1.8.0 #56

Merged

rmconsole-wf added the Merge Requirements Unmet label Mar 16, 2017

rmconsole-wf added Merge Requirements Met and removed Merge Requirements Unmet labels Mar 16, 2017

kaanaksoy-wk mentioned this pull request Mar 24, 2017

UIP-2153: Transition warning sometimes fires when it shouldn't #58

Merged

aaronlademann-wf mentioned this pull request Apr 20, 2017

UIP-2138 Release over_react 1.9.0 #62

Merged

greglittlefield-wf added a commit that referenced this pull request Jun 19, 2020

Merge pull request #55 from Workiva/link-2020/remove-prop-navigation-…

844e3b4

…assist Remove prop navigation assist


		TransitionerComponent transitioner = getDartComponent(renderedInstance);

		transitioner.hide();

Conversation

kaanaksoy-wk commented Mar 9, 2017 • edited by aaronlademann-wf Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Ultimate problem:

How it was fixed:

Testing suggestions:

Potential areas of regression:

Uh oh!

aviary-wf commented Mar 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Raven

Uh oh!

codecov-io commented Mar 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jacehensley-wf left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jacehensley-wf commented Mar 15, 2017

Uh oh!

aaronlademann-wf commented Mar 16, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

teresarevious-wf commented Mar 16, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

kaanaksoy-wk commented Mar 9, 2017 •

edited by aaronlademann-wf

Loading

aviary-wf commented Mar 9, 2017 •

edited

Loading

codecov-io commented Mar 9, 2017 •

edited

Loading

aaronlademann-wf commented Mar 16, 2017 •

edited

Loading