Skip to content
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.

Sign up for GitHub

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

Fix animation duration not consistent in TimeSeriesWidget #214

Merged
merged 4 commits into from
Oct 27, 2021
Merged

Fix animation duration not consistent in TimeSeriesWidget #214

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1a3568d
Animation duration is not consistent in TimeSeriesWidget [sc-188731]
Clebal Oct 26, 2021
f47f6bf
Fix tests
Clebal Oct 26, 2021
72da55e
Merge branch 'master' into bug/sc-188731/animation-duration-is-not-co…
VictorVelarde Oct 26, 2021
ba2954b
Add changelog entry
VictorVelarde Oct 27, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 4 additions & 17 deletions packages/react-ui/__tests__/widgets/TimeSeriesWidgetUI.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,17 +4,17 @@ import TimeSeriesWidgetUI from '../../src/widgets/TimeSeriesWidgetUI/TimeSeriesW
import { getMaterialUIContext, mockEcharts } from './testUtils';
import { GroupDateTypes } from '@carto/react-core';

jest.useFakeTimers();

describe('TimeSeriesWidgetUI', () => {
beforeAll(() => {
mockEcharts.init();
jest.useFakeTimers();
});

afterAll(() => {
mockEcharts.destroy();
});


const DATA = [
{ name: 1514761200000, value: 310 },
{ name: 1515366000000, value: 406 },
Expand Down Expand Up @@ -43,8 +43,6 @@ describe('TimeSeriesWidgetUI', () => {
{ name: 1529272800000, value: 338 }
];

const ANIMATION_STEP = 20000 / DATA.length;

const mandatoryProps = {
stepSize: GroupDateTypes.WEEKS
};
Expand Down Expand Up @@ -199,15 +197,7 @@ describe('TimeSeriesWidgetUI', () => {
<Widget isPlaying={true} isPaused={false} onTimelineUpdate={onTimelineUpdate} />
);

setTimeout(() => expect(onTimelineUpdate).toHaveBeenCalledWith(0), ANIMATION_STEP);
setTimeout(
() => expect(onTimelineUpdate).toHaveBeenCalledWith(1),
ANIMATION_STEP * 2
);
setTimeout(
() => expect(onTimelineUpdate).toHaveBeenCalledWith(2),
ANIMATION_STEP * 3
);
setTimeout(() => expect(onTimelineUpdate).toHaveBeenCalled());
jest.runAllTimers();
});
});
Expand Down Expand Up @@ -240,10 +230,7 @@ describe('TimeSeriesWidgetUI', () => {
/>
);

setTimeout(() => {
expect(onTimeWindowUpdate).toHaveBeenCalledTimes(15);
expect(onStop).toBeCalled();
}, 101);
setTimeout(() => expect(onTimeWindowUpdate).toHaveBeenCalled(), 250);
jest.runAllTimers();
});
});
Expand Down
181 changes: 127 additions & 54 deletions packages/react-ui/src/widgets/TimeSeriesWidgetUI/TimeSeriesWidgetUI.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ import {
capitalize,
makeStyles
} from '@material-ui/core';
import React, { useEffect, useMemo, useRef, useState } from 'react';
import React, { useEffect, useMemo, useRef, useState, useCallback } from 'react';
import TimeSeriesChart from './components/TimeSeriesChart';
import { TimeSeriesProvider, useTimeSeriesContext } from './hooks/TimeSeriesContext';
import { CHART_TYPES } from './utils/constants';
Expand Down Expand Up @@ -38,7 +38,6 @@ function TimeSeriesWidgetUI({
data,
stepSize,
chartType,
duration,
tooltip,
tooltipFormatter,
formatter,
Expand All @@ -55,18 +54,8 @@ function TimeSeriesWidgetUI({
onPause,
onStop
}) {
const [animationStep, setAnimationStep] = useState(0);

// Calculate animation step by duration
useEffect(() => {
if (duration && data.length) {
setAnimationStep(duration / data.length);
}
}, [duration, data, setAnimationStep]);

return (
<TimeSeriesProvider
animationStep={animationStep}
isPlaying={isPlaying}
onPlay={onPlay}
isPaused={isPaused}
Expand Down Expand Up @@ -101,7 +90,6 @@ TimeSeriesWidgetUI.propTypes = {
).isRequired,
stepSize: PropTypes.oneOf(Object.values(GroupDateTypes)).isRequired,
chartType: PropTypes.oneOf(Object.values(CHART_TYPES)),
duration: PropTypes.number,
tooltip: PropTypes.bool,
tooltipFormatter: PropTypes.func,
formatter: PropTypes.func,
Expand All @@ -122,7 +110,6 @@ TimeSeriesWidgetUI.propTypes = {
TimeSeriesWidgetUI.defaultProps = {
data: [],
chartType: CHART_TYPES.LINE,
duration: 20000,
tooltip: true,
tooltipFormatter: defaultTooltipFormatter,
formatter: (value) => value,
Expand Down Expand Up @@ -167,9 +154,9 @@ function TimeSeriesWidgetUIContent({
setTimelinePosition,
setTimeWindow,
stop,
togglePlay,
animationStep
togglePlay
} = useTimeSeriesContext();
const animationRef = useRef({ animationFrameId: null, timeoutId: null });

// If data changes, stop animation. useDidMountEffect is used to avoid
// being executed in the initial rendering because that cause
Expand All @@ -181,52 +168,71 @@ function TimeSeriesWidgetUIContent({
[data]
);

const stopAnimation = () => {
const { animationFrameId, timeoutId } = animationRef.current;
if (animationFrameId) {
window.cancelAnimationFrame(animationFrameId);
}
if (timeoutId) {
clearTimeout(timeoutId);
}
};

const handleStop = useCallback(() => {
stopAnimation();
stop();
}, [stop]);

const handleTogglePlay = () => {
stopAnimation();
togglePlay();
};

// Running timeWindow
useEffect(() => {
if (isPlaying && timeWindow.length === 2) {
if (isPlaying && timeWindow.length === 2 && data.length) {
const timeWindowStep = TIME_WINDOW_STEP_BY_STEP_SIZE[stepSize];
const interval = setInterval(() => {
const msTimeWindowStep = timeWindowStep * 1000;
const newTimeWindow = [
timeWindow[0] + msTimeWindowStep,
timeWindow[1] + msTimeWindowStep
];
if (newTimeWindow[1] > data[data.length - 1].name) {
stop();
clearInterval(interval);
} else {
const msTimeWindowStep = timeWindowStep * 1000;

animateTimeWindow({
data,
timeWindow,
msTimeWindowStep: msTimeWindowStep * speed,
drawFrame: (newTimeWindow) => {
setTimeWindow(newTimeWindow);
}
}, 100 / speed);
return () => clearInterval(interval);
},
onEnd: () => {
// To show the last item, wait a moment
setTimeout(handleStop, 250);
},
animationRef
});

return () => stopAnimation();
}
}, [data, isPlaying, timeWindow, stepSize, setTimeWindow, stop, speed]);
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [data, isPlaying, stepSize, setTimeWindow, handleStop, speed]);

// Running timeline
useEffect(() => {
if (isPlaying && !timeWindow.length) {
const interval = setInterval(() => {
const newTimelinePosition = Math.min(data.length, timelinePosition + 1);
if (isPlaying && newTimelinePosition === data.length) {
clearInterval(interval);
// To show the last item, wait an animationStep
setTimeout(stop, animationStep);
} else {
if (isPlaying && !timeWindow.length && data.length) {
animateTimeline({
speed,
timelinePosition,
data,
drawFrame: (newTimelinePosition) => {
setTimelinePosition(newTimelinePosition);
}
}, animationStep / speed);
return () => clearInterval(interval);
},
onEnd: () => {
setTimeout(handleStop, 250);
},
animationRef
});

return () => stopAnimation();
}
}, [
data,
isPlaying,
animationStep,
speed,
timeWindow.length,
timelinePosition,
stop,
setTimelinePosition
]);
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [data, isPlaying, speed, timeWindow.length, handleStop, setTimelinePosition]);

const currentDate = useMemo(() => {
if (!data.length) {
Expand Down Expand Up @@ -335,7 +341,7 @@ function TimeSeriesWidgetUIContent({
size='small'
color='primary'
disabled={!(isPaused || isPlaying)}
onClick={stop}
onClick={handleStop}
data-testid='stop'
>
<StopIcon />
Expand All @@ -346,7 +352,7 @@ function TimeSeriesWidgetUIContent({
data-testid='play-pause'
size='small'
color='primary'
onClick={togglePlay}
onClick={handleTogglePlay}
>
{isPlaying ? <PauseIcon /> : <PlayIcon />}
</IconButton>
Expand Down Expand Up @@ -470,3 +476,70 @@ function useDidMountEffect(func, deps = []) {
// eslint-disable-next-line react-hooks/exhaustive-deps
}, deps);
}

function animateTimeWindow({
msTimeWindowStep,
timeWindow,
data,
drawFrame,
onEnd,
animationRef
}) {
let currentTimeWindow = timeWindow;

const fireAnimation = () => {
animationRef.current.animationFrameId = window.requestAnimationFrame(animate);
};

const animate = () => {
currentTimeWindow = [
currentTimeWindow[0] + msTimeWindowStep,
currentTimeWindow[1] + msTimeWindowStep
];
if (currentTimeWindow[1] > data[data.length - 1].name) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Clebal what is data[data.length - 1].name) ? it looks something strange, comparing with .name...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, name is the date resulted from aggregation. I used name following the same prop name that in others widgets. But... I can change it if you want.

onEnd();
} else {
drawFrame(currentTimeWindow);
fireAnimation();
}
};

fireAnimation();
}

const MIN_FPS = 2;

function animateTimeline({
speed,
timelinePosition,
data,
drawFrame,
onEnd,
animationRef
}) {
let currentTimeline = timelinePosition;

const fpsToUse =
Math.max(
Math.round(Math.sqrt(data.length) / 2), // FPS based on data length
MIN_FPS // Min FPS
) * speed;

const fireAnimation = () => {
animationRef.current.timeoutId = setTimeout(() => {
animationRef.current.animationFrameId = window.requestAnimationFrame(animate);
}, 1000 / fpsToUse);
};

const animate = () => {
currentTimeline = Math.min(data.length, currentTimeline + 1);
if (currentTimeline === data.length) {
onEnd();
} else {
drawFrame(currentTimeline);
fireAnimation();
}
};

fireAnimation();
}
6 changes: 0 additions & 6 deletions packages/react-ui/src/widgets/TimeSeriesWidgetUI/components/TimeSeriesChart.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,12 +14,6 @@ export default function TimeSeriesChart({
const theme = useTheme();
const [echartsInstance, setEchartInstance] = useState();

// const echartsInstance = useMemo(
// () => chartInstance?.current?.getEchartsInstance(),
// // eslint-disable-next-line react-hooks/exhaustive-deps
// [chartInstance?.current]
// );

const onChartReady = (_echartsInstance) => setEchartInstance(_echartsInstance);

const { processedData, maxValue } = useMemo(() => {
Expand Down
3 changes: 0 additions & 3 deletions packages/react-ui/src/widgets/TimeSeriesWidgetUI/hooks/TimeSeriesContext.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ import React, {
import { getDate, getTimestamp } from '../utils/utilities';

export const TimeSeriesContext = createContext({
animationStep: 0,
isPlaying: false,
setIsPlaying: (value) => {},
onPlay: () => {},
Expand All @@ -32,7 +31,6 @@ export function useTimeSeriesContext() {

export function TimeSeriesProvider({
children,
animationStep,
isPlaying,
isPaused,
onPlay,
Expand Down Expand Up @@ -144,7 +142,6 @@ export function TimeSeriesProvider({
onTimeWindowUpdate,
togglePlay,
stop,
animationStep
}}
>
{children}
Expand Down
7 changes: 3 additions & 4 deletions packages/react-ui/src/widgets/TimeSeriesWidgetUI/hooks/useTimeSeriesInteractivity.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,7 @@ export default function useTimeSeriesInteractivity({ echartsInstance, data }) {
timeWindow,
setTimeWindow,
setTimelinePosition,
stop,
animationStep
stop
} = useTimeSeriesContext();

const [isMarkLineSelected, setIsMarkLineSelected] = useState(false);
Expand Down Expand Up @@ -196,7 +195,7 @@ export default function useTimeSeriesInteractivity({ echartsInstance, data }) {
xAxis !== undefined && {
symbol: ['none', 'none'],
animationDuration: 100,
animationDurationUpdate: Math.min(300, animationStep / 2),
animationDurationUpdate: 150,
animationEasingUpdate: 'linear',
data:
isPaused || isPlaying
Expand All @@ -223,7 +222,7 @@ export default function useTimeSeriesInteractivity({ echartsInstance, data }) {
: []
}
);
}, [isPaused, isPlaying, data, theme, animationStep, timelinePosition, timeWindow]);
}, [isPaused, isPlaying, data, theme, timelinePosition, timeWindow]);

// markArea in echarts
const timeWindowOptions = useMemo(
Expand Down
3 changes: 0 additions & 3 deletions packages/react-ui/storybook/stories/widgetsUI/TimeSeriesWidgetUI.stories.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,9 +93,6 @@ const options = {
}, {})
}
},
duration: {
control: { type: 'number' }
},
tooltip: {
control: { type: 'boolean' }
},
Expand Down
Loading