From 0217f1980d65cadfa2209548ea1f3e98e77c8cd8 Mon Sep 17 00:00:00 2001 From: Michael Telatynski <7t3chguy@gmail.com> Date: Wed, 24 Aug 2022 16:34:58 +0100 Subject: [PATCH 01/10] Fix useSmoothAnimation enablement not working properly by getting rid of it Passing duration=0 is more logical and less superfluous --- src/hooks/useSmoothAnimation.ts | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/hooks/useSmoothAnimation.ts b/src/hooks/useSmoothAnimation.ts index 8d652f32579..743018aba8a 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; } From 65918f09dc5b29d510d4678efd7d0b95eb90036d Mon Sep 17 00:00:00 2001 From: Michael Telatynski <7t3chguy@gmail.com> Date: Wed, 24 Aug 2022 16:36:29 +0100 Subject: [PATCH 02/10] Refactor UploadBar to handle state more correctly --- src/components/structures/UploadBar.tsx | 57 ++++++++++++++++-------- src/dispatcher/payloads/UploadPayload.ts | 2 +- 2 files changed, 39 insertions(+), 20 deletions(-) diff --git a/src/components/structures/UploadBar.tsx b/src/components/structures/UploadBar.tsx index e3dc77a4665..2215b1274c5 100644 --- a/src/components/structures/UploadBar.tsx +++ b/src/components/structures/UploadBar.tsx @@ -17,17 +17,17 @@ 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 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 { UploadPayload } from "../../dispatcher/payloads/UploadPayload"; interface IProps { room: Room; @@ -35,11 +35,14 @@ 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 { +export default class UploadBar extends React.PureComponent { static contextType = MatrixClientContext; private dispatcherRef: string; @@ -50,8 +53,7 @@ export default class UploadBar extends React.Component { // 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() { @@ -69,45 +71,62 @@ export default class UploadBar extends React.Component { return uploads.filter(u => u.roomId === this.props.room.roomId); } - private onAction = (payload: ActionPayload) => { + 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: UploadPayload) => { 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 }); + this.setState(this.calculateState()); break; } + case Action.UploadProgress: + if (payload.upload.promise === this.state.currentPromise) { + this.setState({ + currentLoaded: payload.upload.loaded, + currentTotal: payload.upload.total, + }); + } + break; } }; - 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/dispatcher/payloads/UploadPayload.ts b/src/dispatcher/payloads/UploadPayload.ts index 023bd5403ce..7db4a4a4d74 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. */ From 82a99474b0938bcc8a8b839df221d405947788a1 Mon Sep 17 00:00:00 2001 From: Michael Telatynski <7t3chguy@gmail.com> Date: Wed, 24 Aug 2022 16:36:51 +0100 Subject: [PATCH 03/10] Change ProgressBar to new useSmoothAnimation signature and default animated to true for consistency --- src/components/views/elements/ProgressBar.tsx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/components/views/elements/ProgressBar.tsx b/src/components/views/elements/ProgressBar.tsx index af06f579ead..2759846ffe4 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 ; }; From a0b04268a0f542b0cdd472f6fd261d023ebf9894 Mon Sep 17 00:00:00 2001 From: Michael Telatynski <7t3chguy@gmail.com> Date: Wed, 24 Aug 2022 16:49:16 +0100 Subject: [PATCH 04/10] Add type guard --- src/components/structures/UploadBar.tsx | 33 +++++++++++-------------- 1 file changed, 15 insertions(+), 18 deletions(-) diff --git a/src/components/structures/UploadBar.tsx b/src/components/structures/UploadBar.tsx index 2215b1274c5..26e68189d8f 100644 --- a/src/components/structures/UploadBar.tsx +++ b/src/components/structures/UploadBar.tsx @@ -27,6 +27,7 @@ import ProgressBar from "../views/elements/ProgressBar"; 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 { @@ -42,6 +43,16 @@ interface IState { countFiles: number; } +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; @@ -82,24 +93,10 @@ export default class UploadBar extends React.PureComponent { }; } - private onAction = (payload: UploadPayload) => { - switch (payload.action) { - case Action.UploadStarted: - case Action.UploadFinished: - case Action.UploadCanceled: - case Action.UploadFailed: { - if (!this.mounted) return; - this.setState(this.calculateState()); - break; - } - case Action.UploadProgress: - if (payload.upload.promise === this.state.currentPromise) { - this.setState({ - currentLoaded: payload.upload.loaded, - currentTotal: payload.upload.total, - }); - } - break; + private onAction = (payload: ActionPayload) => { + if (!this.mounted) return; + if (isUploadPayload(payload)) { + this.setState(this.calculateState()); } }; From 2b304d66d90dc7d68e6d8595952f293176186053 Mon Sep 17 00:00:00 2001 From: Michael Telatynski <7t3chguy@gmail.com> Date: Wed, 24 Aug 2022 16:56:56 +0100 Subject: [PATCH 05/10] Make types stricter --- src/components/structures/UploadBar.tsx | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/components/structures/UploadBar.tsx b/src/components/structures/UploadBar.tsx index 26e68189d8f..83b1f124cff 100644 --- a/src/components/structures/UploadBar.tsx +++ b/src/components/structures/UploadBar.tsx @@ -18,6 +18,7 @@ import React from 'react'; import { Room } from "matrix-js-sdk/src/models/room"; import filesize from "filesize"; import { IAbortablePromise, IEventRelation } from 'matrix-js-sdk/src/matrix'; +import { Optional } from "matrix-events-sdk"; import ContentMessages from '../../ContentMessages'; import dis from "../../dispatcher/dispatcher"; @@ -56,8 +57,8 @@ function isUploadPayload(payload: ActionPayload): payload is UploadPayload { 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); @@ -74,7 +75,7 @@ export default class UploadBar extends React.PureComponent { componentWillUnmount() { this.mounted = false; - dis.unregister(this.dispatcherRef); + dis.unregister(this.dispatcherRef!); } private getUploadsInRoom(): IUpload[] { From b89749c5cfe9dd5f96d156f0227962f3f77df094 Mon Sep 17 00:00:00 2001 From: Michael Telatynski <7t3chguy@gmail.com> Date: Thu, 25 Aug 2022 12:10:43 +0100 Subject: [PATCH 06/10] Write tests for the ProgressBar component --- .../views/elements/ProgressBar-test.tsx | 53 +++++++++++++++++++ 1 file changed, 53 insertions(+) create mode 100644 test/components/views/elements/ProgressBar-test.tsx diff --git a/test/components/views/elements/ProgressBar-test.tsx b/test/components/views/elements/ProgressBar-test.tsx new file mode 100644 index 00000000000..da00aa0f7c8 --- /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); + }); +}); From da5c36f5ef8e72bbb63a13d2df0c7ecb053496a7 Mon Sep 17 00:00:00 2001 From: Michael Telatynski <7t3chguy@gmail.com> Date: Thu, 25 Aug 2022 12:23:50 +0100 Subject: [PATCH 07/10] Make the new test conform to tsc --strict --- test/components/views/elements/ProgressBar-test.tsx | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/components/views/elements/ProgressBar-test.tsx b/test/components/views/elements/ProgressBar-test.tsx index da00aa0f7c8..320304fb76b 100644 --- a/test/components/views/elements/ProgressBar-test.tsx +++ b/test/components/views/elements/ProgressBar-test.tsx @@ -22,7 +22,7 @@ jest.useFakeTimers(); describe("", () => { it("works when animated", () => { const { container, rerender } = render(); - const progress = container.querySelector("progress"); + const progress = container.querySelector("progress")!; // The animation always starts from 0 expect(progress.value).toBe(0); @@ -43,7 +43,7 @@ describe("", () => { it("works when not animated", () => { const { container, rerender } = render(); - const progress = container.querySelector("progress"); + const progress = container.querySelector("progress")!; // Without animation all positional updates are immediate, not requiring timers to run expect(progress.position).toBe(0.5); From ad62b787f99600eb2af0e77b7714c16d27c72cb5 Mon Sep 17 00:00:00 2001 From: Michael Telatynski <7t3chguy@gmail.com> Date: Thu, 25 Aug 2022 15:28:13 +0100 Subject: [PATCH 08/10] Update UploadBar.tsx --- src/components/structures/UploadBar.tsx | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/components/structures/UploadBar.tsx b/src/components/structures/UploadBar.tsx index 83b1f124cff..685020d5f7f 100644 --- a/src/components/structures/UploadBar.tsx +++ b/src/components/structures/UploadBar.tsx @@ -86,10 +86,10 @@ export default class UploadBar extends React.PureComponent { private calculateState(): IState { const [currentUpload, ...otherUploads] = this.getUploadsInRoom(); return { - currentFile: currentUpload.fileName, - currentPromise: currentUpload.promise, - currentLoaded: currentUpload.loaded, - currentTotal: currentUpload.total, + currentFile: currentUpload?.fileName, + currentPromise: currentUpload?.promise, + currentLoaded: currentUpload?.loaded, + currentTotal: currentUpload?.total, countFiles: otherUploads.length + 1, }; } From a17caa221cca571ad25f922ee297f7aa89b2abb3 Mon Sep 17 00:00:00 2001 From: Michael Telatynski <7t3chguy@gmail.com> Date: Thu, 25 Aug 2022 15:28:37 +0100 Subject: [PATCH 09/10] Update UploadBar.tsx --- src/components/structures/UploadBar.tsx | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/src/components/structures/UploadBar.tsx b/src/components/structures/UploadBar.tsx index 685020d5f7f..be5d3b0fa81 100644 --- a/src/components/structures/UploadBar.tsx +++ b/src/components/structures/UploadBar.tsx @@ -37,10 +37,10 @@ interface IProps { } interface IState { - currentFile: string; - currentPromise: IAbortablePromise; - currentLoaded: number; - currentTotal: number; + currentFile?: string; + currentPromise?: IAbortablePromise; + currentLoaded?: number; + currentTotal?: number; countFiles: number; } From 592efabaebd66b6cbc32b7982702e4c07bffe1bf Mon Sep 17 00:00:00 2001 From: Michael Telatynski <7t3chguy@gmail.com> Date: Thu, 25 Aug 2022 15:40:16 +0100 Subject: [PATCH 10/10] Update UploadBar.tsx --- src/components/structures/UploadBar.tsx | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/components/structures/UploadBar.tsx b/src/components/structures/UploadBar.tsx index be5d3b0fa81..b2c7544f1fd 100644 --- a/src/components/structures/UploadBar.tsx +++ b/src/components/structures/UploadBar.tsx @@ -103,7 +103,7 @@ export default class UploadBar extends React.PureComponent { private onCancelClick = (ev: ButtonEvent) => { ev.preventDefault(); - ContentMessages.sharedInstance().cancelUpload(this.state.currentPromise, this.context); + ContentMessages.sharedInstance().cancelUpload(this.state.currentPromise!, this.context); }; render() { @@ -119,12 +119,12 @@ export default class UploadBar extends React.PureComponent { }, ); - const uploadSize = filesize(this.state.currentTotal); + const uploadSize = filesize(this.state.currentTotal!); return (
{ uploadText } ({ uploadSize })
- +
); }