swimlane / ngx-charts

:bar_chart: Declarative Charting Framework for Angular
https://swimlane.github.io/ngx-charts/
MIT License
4.28k stars 1.15k forks source link

Number Card: animations would disturb normal value display when input results update within fixed duration #1516

Open hijiangtao opened 3 years ago

hijiangtao commented 3 years ago

Describe the bug

When we use <ngx-charts-number-card> we need to specify results property as chart data, and animations property is default to true to enable animation effects. However, if we change results very quickly (suppose we change value inside results from 0 to 100), the card would remain to display 0 rather the 100 we want.

To Reproduce Steps to reproduce the behavior (GIST):

  1. Define a number card component, define data as an input property to update component and set animations to true ( https://gist.github.com/hijiangtao/5ebc7559d7fd781320258465341b7eac#file-ux-text-card-component-ts );
  2. Use the defined component and set it's initial data to whatever you like ( https://gist.github.com/hijiangtao/5ebc7559d7fd781320258465341b7eac#file-index-ts-L12-L15 );
  3. Change the data you defined in step 2 to another one in less than 1000ms (you can do it with setTimeout API)
  4. See the changes in Web (Animation runs normally, but the result would stop at the old value, rather than the new one)

Demo

https://gist.github.com/hijiangtao/5ebc7559d7fd781320258465341b7eac

ngx-charts version

v16.0.0

Additional context

Here's the problem I found in codes, as well as my solution to it.

It's a bug caused by animations property mixed with initialized property, since the animation duration is set to a fixed time as 1000ms.

Looking into update() part in https://github.com/swimlane/ngx-charts/blob/48dc53364602cc08269ee0fa0ae36e386c040b20/projects/swimlane/ngx-charts/src/lib/number-card/card.component.ts#L148-L154

it would check if we initialize chart (as well as hasValue) and then call startCount in 20ms later (with setTimeout API), in which we establish a requestAnimationFrame calling (fixed to 1000ms as the third input parameter to count method):

https://github.com/swimlane/ngx-charts/blob/48dc53364602cc08269ee0fa0ae36e386c040b20/projects/swimlane/ngx-charts/src/lib/number-card/card.component.ts#L184

but the problem is if we do update the input results within 1000ms (which is expected as the end of animation), we may have the animations already start but with following condition to be false:

https://github.com/swimlane/ngx-charts/blob/48dc53364602cc08269ee0fa0ae36e386c040b20/projects/swimlane/ngx-charts/src/lib/number-card/card.component.ts#L166

then it wouldn't cancel this.animationReq so this.value would become unstable in rendering html:

https://github.com/swimlane/ngx-charts/blob/48dc53364602cc08269ee0fa0ae36e386c040b20/projects/swimlane/ngx-charts/src/lib/number-card/card.component.ts#L61

In other words, we can change this.value in many places, and trigger Angular change detection in both this.scaleText():

https://github.com/swimlane/ngx-charts/blob/48dc53364602cc08269ee0fa0ae36e386c040b20/projects/swimlane/ngx-charts/src/lib/number-card/card.component.ts#L205

and requestAnimationFrame's callback:

https://github.com/swimlane/ngx-charts/blob/48dc53364602cc08269ee0fa0ae36e386c040b20/projects/swimlane/ngx-charts/src/lib/number-card/card.component.ts#L180

so updated results may get updated first in this.scaleText() within update() calling, but get back to old value with animation still runs with callback, since we update this.initialized immediately after we start the counting animation.

A solution and it's expected result is to set this.initialized to true only if the animation ends, which is 1000ms later, and to do so, we may need another parameter to count to specify another callback when this condition to evaluate to false:

https://github.com/swimlane/ngx-charts/blob/48dc53364602cc08269ee0fa0ae36e386c040b20/projects/swimlane/ngx-charts/src/lib/common/count/count.helper.ts#L44

I would like to PR for it if you confirm this bug.

Selindek commented 2 years ago

Any progress on this issue?

I wasted hours yesterday becasue of this bug...