Add a delay before displaying the running man icon. #9732
Conversation
sfc-gh-lwilby
changed the title
sfc-gh-lwilby
force-pushed
the
feature-running-man-delay
branch
from
fff2c25 to
f906c26
Compare
sfc-gh-lwilby
changed the title
sfc-gh-lwilby
added
security-assessment-completed
sfc-gh-lwilby
force-pushed
the
feature-running-man-delay
branch
from
e836093 to
5e7a208
Compare
There was a problem hiding this comment.
in order to double check that something will happen after a delay, it could make sense to advance the timer after the expect(icon).not.toBeInTheDocument() check and ensure that afterwards the running man is shown. Otherwise the test could easily be false positive by never render any running gif. What do you think?
There was a problem hiding this comment.
Yeah, I agree.
Yesterday I was playing around with the different fake timer options and I found that if I advanced the timer, I still needed to use findByRole. But, I didn't like this because findByRole will succeed with or without the timer being advanced since it waits for the element to appear. So, I decided to just leave the tests above testing that the running man icon does appear.
I've just tried again adding it as a second step in this test, and in this case queryByRole is working. Based on trial and error it looks like the render must be called before advancing the timer and yesterday I was testing only the appearing case separately from the not appearing and advanced the timer at the top of the test.
There was a problem hiding this comment.
nit: based on the comment, I think a better variable name would be delayShowingRunningMan. Because if the showRunningMan is false, it is shown right away instead of never, right?
There was a problem hiding this comment.
Hm, after looking at the rest of the code, it seems like the variable name is correct but the comment is confusing. I think the comment should just be: If state is true, the running man animation is shown or similar because the variable itself is controlled by the delay, but not directly related.
There was a problem hiding this comment.
Thanks, this comment is unclear. I dropped the part about the delay.
raethlein
left a comment
raethlein
left a comment
There was a problem hiding this comment.
LGTM! I have just left a few minor comments.
Also for context, we have discussed offline to migrate the component to a functional one after the PR is merged.
sfc-gh-lwilby
force-pushed
the
feature-running-man-delay
branch
from
d7ab74e to
89117a6
Compare
## Describe your changes The running man animation appears while the script re-run is being performed and for re-runs that complete quickly it introduces a flicker and creates the perception of a longer wait. This PR introduces a delay when showing the running man animation. ## GitHub Issue Link (if applicable) ## Testing Plan - Unit Tests (one added to confirm not visible without delay, plus coverage by existing running man tests) - Manual testing by PM and Engineer to confirm expected visual behaviour. --- **Contribution License Agreement** By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.
Describe your changes
The running man animation appears while the script re-run is being performed and for re-runs that complete quickly it introduces a flicker and creates the perception of a longer wait. This PR introduces a delay when showing the running man animation.
GitHub Issue Link (if applicable)
Testing Plan
Contribution License Agreement
By submitting this pull request you agree that all contributions to this project are made under the Apache 2.0 license.