From 9b99c967f4d65e3446a18a677fa4010612958d90 Mon Sep 17 00:00:00 2001 From: Michael Telatynski <7t3chguy@gmail.com> Date: Thu, 25 Aug 2022 16:39:00 +0100 Subject: [PATCH] Fix progress bar regression throughout the app (#9219) * Fix useSmoothAnimation enablement not working properly by getting rid of it Passing duration=0 is more logical and less superfluous * Refactor UploadBar to handle state more correctly * Change ProgressBar to new useSmoothAnimation signature and default animated to true for consistency * Add type guard * Make types stricter * Write tests for the ProgressBar component * Make the new test conform to tsc --strict * Update UploadBar.tsx * Update UploadBar.tsx * Update UploadBar.tsx --- src/components/structures/UploadBar.tsx | 75 ++++++++++++------- src/components/views/elements/ProgressBar.tsx | 4 +- src/dispatcher/payloads/UploadPayload.ts | 2 +- src/hooks/useSmoothAnimation.ts | 8 +- .../views/elements/ProgressBar-test.tsx | 53 +++++++++++++ 5 files changed, 105 insertions(+), 37 deletions(-) create mode 100644 test/components/views/elements/ProgressBar-test.tsx diff --git a/src/components/structures/UploadBar.tsx b/src/components/structures/UploadBar.tsx index e3dc77a466..b2c7544f1f 100644 --- a/src/components/structures/UploadBar.tsx +++ b/src/components/structures/UploadBar.tsx @@ -17,17 +17,19 @@ limitations under the License. import React from 'react'; import { Room } from "matrix-js-sdk/src/models/room"; import filesize from "filesize"; -import { IEventRelation } from 'matrix-js-sdk/src/matrix'; +import { IAbortablePromise, IEventRelation } from 'matrix-js-sdk/src/matrix'; +import { Optional } from "matrix-events-sdk"; import ContentMessages from '../../ContentMessages'; import dis from "../../dispatcher/dispatcher"; import { _t } from '../../languageHandler'; -import { ActionPayload } from "../../dispatcher/payloads"; import { Action } from "../../dispatcher/actions"; import ProgressBar from "../views/elements/ProgressBar"; -import AccessibleButton from "../views/elements/AccessibleButton"; +import AccessibleButton, { ButtonEvent } from "../views/elements/AccessibleButton"; import { IUpload } from "../../models/IUpload"; import MatrixClientContext from "../../contexts/MatrixClientContext"; +import { ActionPayload } from '../../dispatcher/payloads'; +import { UploadPayload } from "../../dispatcher/payloads/UploadPayload"; interface IProps { room: Room; @@ -35,23 +37,35 @@ interface IProps { } interface IState { - currentUpload?: IUpload; - uploadsHere: IUpload[]; + currentFile?: string; + currentPromise?: IAbortablePromise; + currentLoaded?: number; + currentTotal?: number; + countFiles: number; } -export default class UploadBar extends React.Component { +function isUploadPayload(payload: ActionPayload): payload is UploadPayload { + return [ + Action.UploadStarted, + Action.UploadProgress, + Action.UploadFailed, + Action.UploadFinished, + Action.UploadCanceled, + ].includes(payload.action as Action); +} + +export default class UploadBar extends React.PureComponent { static contextType = MatrixClientContext; - private dispatcherRef: string; - private mounted: boolean; + private dispatcherRef: Optional; + private mounted = false; constructor(props) { super(props); // Set initial state to any available upload in this room - we might be mounting // earlier than the first progress event, so should show something relevant. - const uploadsHere = this.getUploadsInRoom(); - this.state = { currentUpload: uploadsHere[0], uploadsHere }; + this.state = this.calculateState(); } componentDidMount() { @@ -61,7 +75,7 @@ export default class UploadBar extends React.Component { componentWillUnmount() { this.mounted = false; - dis.unregister(this.dispatcherRef); + dis.unregister(this.dispatcherRef!); } private getUploadsInRoom(): IUpload[] { @@ -69,45 +83,48 @@ export default class UploadBar extends React.Component { return uploads.filter(u => u.roomId === this.props.room.roomId); } + private calculateState(): IState { + const [currentUpload, ...otherUploads] = this.getUploadsInRoom(); + return { + currentFile: currentUpload?.fileName, + currentPromise: currentUpload?.promise, + currentLoaded: currentUpload?.loaded, + currentTotal: currentUpload?.total, + countFiles: otherUploads.length + 1, + }; + } + private onAction = (payload: ActionPayload) => { - switch (payload.action) { - case Action.UploadStarted: - case Action.UploadProgress: - case Action.UploadFinished: - case Action.UploadCanceled: - case Action.UploadFailed: { - if (!this.mounted) return; - const uploadsHere = this.getUploadsInRoom(); - this.setState({ currentUpload: uploadsHere[0], uploadsHere }); - break; - } + if (!this.mounted) return; + if (isUploadPayload(payload)) { + this.setState(this.calculateState()); } }; - private onCancelClick = (ev) => { + private onCancelClick = (ev: ButtonEvent) => { ev.preventDefault(); - ContentMessages.sharedInstance().cancelUpload(this.state.currentUpload.promise, this.context); + ContentMessages.sharedInstance().cancelUpload(this.state.currentPromise!, this.context); }; render() { - if (!this.state.currentUpload) { + if (!this.state.currentFile) { return null; } // MUST use var name 'count' for pluralization to kick in const uploadText = _t( "Uploading %(filename)s and %(count)s others", { - filename: this.state.currentUpload.fileName, - count: this.state.uploadsHere.length - 1, + filename: this.state.currentFile, + count: this.state.countFiles - 1, }, ); - const uploadSize = filesize(this.state.currentUpload.total); + const uploadSize = filesize(this.state.currentTotal!); return (
{ uploadText } ({ uploadSize })
- +
); } diff --git a/src/components/views/elements/ProgressBar.tsx b/src/components/views/elements/ProgressBar.tsx index af06f579ea..2759846ffe 100644 --- a/src/components/views/elements/ProgressBar.tsx +++ b/src/components/views/elements/ProgressBar.tsx @@ -25,10 +25,10 @@ interface IProps { } const PROGRESS_BAR_ANIMATION_DURATION = 300; -const ProgressBar: React.FC = ({ value, max, animated }) => { +const ProgressBar: React.FC = ({ value, max, animated = true }) => { // Animating progress bars via CSS transition isn’t possible in all of our supported browsers yet. // As workaround, we’re using animations through JS requestAnimationFrame - const currentValue = useSmoothAnimation(0, value, PROGRESS_BAR_ANIMATION_DURATION, animated); + const currentValue = useSmoothAnimation(0, value, animated ? PROGRESS_BAR_ANIMATION_DURATION : 0); return ; }; diff --git a/src/dispatcher/payloads/UploadPayload.ts b/src/dispatcher/payloads/UploadPayload.ts index 023bd5403c..7db4a4a4d7 100644 --- a/src/dispatcher/payloads/UploadPayload.ts +++ b/src/dispatcher/payloads/UploadPayload.ts @@ -18,7 +18,7 @@ import { ActionPayload } from "../payloads"; import { Action } from "../actions"; import { IUpload } from "../../models/IUpload"; -interface UploadPayload extends ActionPayload { +export interface UploadPayload extends ActionPayload { /** * The upload with fields representing the new upload state. */ diff --git a/src/hooks/useSmoothAnimation.ts b/src/hooks/useSmoothAnimation.ts index 8d652f3257..743018aba8 100644 --- a/src/hooks/useSmoothAnimation.ts +++ b/src/hooks/useSmoothAnimation.ts @@ -30,14 +30,12 @@ const debuglog = (...args: any[]) => { * Utility function to smoothly animate to a certain target value * @param initialValue Initial value to be used as initial starting point * @param targetValue Desired value to animate to (can be changed repeatedly to whatever is current at that time) - * @param duration Duration that each animation should take - * @param enabled Whether the animation should run or not + * @param duration Duration that each animation should take, specify 0 to skip animating */ export function useSmoothAnimation( initialValue: number, targetValue: number, duration: number, - enabled: boolean, ): number { const state = useRef<{ timestamp: DOMHighResTimeStamp | null, value: number }>({ timestamp: null, @@ -79,7 +77,7 @@ export function useSmoothAnimation( [currentStepSize, targetValue], ); - useAnimation(enabled, update); + useAnimation(duration > 0, update); - return currentValue; + return duration > 0 ? currentValue : targetValue; } diff --git a/test/components/views/elements/ProgressBar-test.tsx b/test/components/views/elements/ProgressBar-test.tsx new file mode 100644 index 0000000000..320304fb76 --- /dev/null +++ b/test/components/views/elements/ProgressBar-test.tsx @@ -0,0 +1,53 @@ +/* +Copyright 2022 The Matrix.org Foundation C.I.C. + +Licensed under the Apache License, Version 2.0 (the "License"); +you may not use this file except in compliance with the License. +You may obtain a copy of the License at + http://www.apache.org/licenses/LICENSE-2.0 +Unless required by applicable law or agreed to in writing, software +distributed under the License is distributed on an "AS IS" BASIS, +WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +See the License for the specific language governing permissions and +limitations under the License. +*/ + +import React from "react"; +import { act, render } from "@testing-library/react"; + +import ProgressBar from "../../../../src/components/views/elements/ProgressBar"; + +jest.useFakeTimers(); + +describe("", () => { + it("works when animated", () => { + const { container, rerender } = render(); + const progress = container.querySelector("progress")!; + + // The animation always starts from 0 + expect(progress.value).toBe(0); + + // Await the animation to conclude to our initial value of 50 + act(() => { jest.runAllTimers(); }); + expect(progress.position).toBe(0.5); + + // Move the needle to 80% + rerender(); + expect(progress.position).toBe(0.5); + + // Let the animaiton run a tiny bit, assert it has moved from where it was to where it needs to go + act(() => { jest.advanceTimersByTime(150); }); + expect(progress.position).toBeGreaterThan(0.5); + expect(progress.position).toBeLessThan(0.8); + }); + + it("works when not animated", () => { + const { container, rerender } = render(); + const progress = container.querySelector("progress")!; + + // Without animation all positional updates are immediate, not requiring timers to run + expect(progress.position).toBe(0.5); + rerender(); + expect(progress.position).toBe(0.8); + }); +});