WRQ-24303: Fix `marqueeDecorator` and `marqueeController` to start animation properly when content changed

[juwonjeong]

Enact-DCO-1.0-Signed-off-by: Juwon Jeong (juwon.jeong@lge.com)

Checklist

• [x] I have read and understand the contribution guide
• [x] A CHANGELOG entry is included
• [ ] At least one test case is included for this feature or bug fix
• [x] Documentation was added or is not needed
• [ ] This is an API breaking change

Issue Resolved / Feature Added

When is wrapped with MarqueeController to be synchronized and marqueeOn props is 'render' and the content changes, MarqueeDecorator cannot start the marquee animation.

Resolution

MarqueeDecorator.componentDidUpdate() called when content changed. As children is changed, this.invalidateMetrics(), this.cancelAnimation() are called and forceRestartMarquee variable set to true in componentDidUpdate function,

this.tryStartingAnimation() function is called because forceRestartMarquee is true. However, In tryStartingAnimation(), it returns without calling startAnimation() function becuase this.timerState is 1(=TimerState.START_PENDING). this.timerState is set to 1 in MarqueeDecorator.start() function which is called before componentDidUpdate.

So I added retryStartingAnimation flag in MarqueeDecorator.cancelAnimation() to check whether we need to restartAnimation after cancelJob completes.

Additional Considerations

In additional, I think handleComplete in useMarqueeController needs to be fixed too. When is wrapped with a MarqueeController, I found that the MarqueeDecorator.start function is constantly being called, even though content is short that doesn't need to be animate.

Links

WRQ-24303

Comments

codecov[bot] commented 4 months ago

Codecov Report

Attention: Patch coverage is 61.53846% with 5 lines in your changes missing coverage. Please review.

Project coverage is 82.45%. Comparing base (1e444a8) to head (bbb71ff).

Files	Patch %	Lines
packages/ui/Marquee/useMarqueeController.js	57.14%	2 Missing and 1 partial :warning:
packages/ui/Marquee/MarqueeDecorator.js	66.66%	2 Missing :warning:

Additional details and impacted files

```diff @@ Coverage Diff @@ ## develop #3247 +/- ## =========================================== - Coverage 82.48% 82.45% -0.04% =========================================== Files 157 157 Lines 7247 7251 +4 Branches 1919 1923 +4 =========================================== + Hits 5978 5979 +1 - Misses 998 1000 +2 - Partials 271 272 +1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

juwonjeong commented 4 months ago

Since the patch is implemented in cancelJob which is called with setTimeout, I cannot add unit test of it.

0x64 commented 4 months ago

Let's add 'restart' function to MarqueeDecorator.js that simply calls 'restartAnimation' for consistent API sets. Looks great except this.

juwonjeong commented 4 months ago

It's fixed :)

0x64 commented 4 months ago

Checked codecov results and it looks okay.

• © Githubissues.
• Githubissues is a development platform for aggregating issues.

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 61.53846% with 5 lines in your changes missing coverage. Please review.

Project coverage is 82.45%. Comparing base (1e444a8) to head (bbb71ff).

Files	Patch %	Lines
packages/ui/Marquee/useMarqueeController.js	57.14%	2 Missing and 1 partial :warning:
packages/ui/Marquee/MarqueeDecorator.js	66.66%	2 Missing :warning:

Additional details and impacted files

```diff @@ Coverage Diff @@ ## develop #3247 +/- ## =========================================== - Coverage 82.48% 82.45% -0.04% =========================================== Files 157 157 Lines 7247 7251 +4 Branches 1919 1923 +4 =========================================== + Hits 5978 5979 +1 - Misses 998 1000 +2 - Partials 271 272 +1 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[juwonjeong]

Since the patch is implemented in cancelJob which is called with setTimeout, I cannot add unit test of it.

[0x64]

Let's add 'restart' function to MarqueeDecorator.js that simply calls 'restartAnimation' for consistent API sets. Looks great except this.

[juwonjeong]

It's fixed :)

[0x64]

Checked codecov results and it looks okay.