Feature/record movie UI #483

interim17 · 2024-03-26T17:45:17Z

Time estimate or Size

Review time: medium

Problem

Closes #327

Solution

RecordMovieComponent manages the modal and button, retrieves and formats the various display icons and text, defines the downloading function, and calls the viewer handlers it receives.

Modal is trim, no logic.

Minimized storing state in ViewerPanel: only holding the url for the blob.

New feature (non-breaking change which adds functionality)

Steps to Verify:

On supported browsers:

Load a trajectory, click record, confirm that recording indicator shows up next to button.
During playback the record button should pulse red unless hovering, when it should show a red square.
Click stop recording, and then confirm download via the modal.
Check that the downloaded movie is playable.

On unsupported browsers (Firefox, and apparently sometimes Safari):

Confirm that the button has disabled styling and does nothing when clicked.

Screenshots (optional):

… and title in download handler

interim17 · 2024-03-26T17:49:07Z

src/components/ViewportButton/style.css

+    color: var(--dark-theme-viewport-button-disabled-color);
+}
+
+


These are not general to any other viewport buttons but need this disabled state so they are here, see note above for my questions.

interim17 · 2024-03-26T17:50:36Z

src/components/RecordMoviesComponent/index.tsx

+    const startRecordIcon = (
+        <span className={styles.iconContainer}>
+            <span
+                className={classNames("icon-moon", "record-icon-circle")}
+            ></span>
+            <span
+                className={classNames("icon-moon", "record-icon-ring")}
+            ></span>
+        </span>
+    );


To do multicolor icons via icomoon requires stacking glyphs. This was the best I could come up with, but I'm open to feedback if there is a better way!

Debated defining this in Icons.tsx with inline styles and importing from there, but that file typically doesn't handle any thing related to icomoon icons... Style rules for this icon also got a bit spread out, some in primary stylesheet, some here, and some in viewport button. Not ideal? Could consolidate somewhat by moving most of it to ViewportButton stylesheet but it seems a bit case-specific for that, so I only put the things that relied on disabled state there. This works, but I'm definitely down to refactor/move things around.

interim17 · 2024-03-26T17:52:20Z

src/components/RecordMoviesComponent/index.tsx

+    const useFormattedDuration = () => {
+        const [formattedDuration, setFormattedDuration] = useState("");
+
+        useEffect(() => {
+            const video = document.createElement("video");
+            video.src = movieUrl;
+            video.onloadedmetadata = () => {
+                const durationInSeconds = video.duration;
+                const minutes = Math.floor((durationInSeconds % 3600) / 60);
+                const secs = Math.ceil(durationInSeconds) % 60;
+
+                const minutesStr = minutes.toString().padStart(2, "0");
+                const secondsStr = secs.toString().padStart(2, "0");
+
+                setFormattedDuration(`${minutesStr}:${secondsStr}`);
+                video.src = "";
+                video.load();
+            };
+        }, [movieUrl]);
+
+        return formattedDuration;
+    };


onloadedmetadata is async hence the need for a hook, Promise or callback. Code was less complicated if this is handled up in ViewerPanel and store in state there but that file is already very cluttered with many concerns...

could you just move this out into its own file?

interim17 · 2024-03-26T17:52:58Z

src/components/ViewportButton/index.tsx

+    const getTooltip = () =>
+        !disabled || tooltipWhenDisabled ? tooltipText : "";
+
+    const getClickHandler = () => (!disabled ? clickHandler : undefined);
+


Needed these tweaks to manage the new case of a button that is disabled but has a tooltip.

commit f622cae Author: Joe Heffernan <interim17@gmail.com> Date: Tue Apr 2 09:12:34 2024 -0700 Fix/update custom modal (#482) * use custom modal wrapper for more layout and styling * remove duplicate and unused colors * remove more duplicated colors * remove comments * remove unused import from starcheckbox * simplify time input css selector * fix spelling errors in "shareable" * omit unused ModalProps from CustomModalProps * add comment to share modal input styles commit 0af4198 Author: Joe Heffernan <interim17@gmail.com> Date: Mon Apr 1 09:48:40 2024 -0700 Fix/header buttons (#484) * custom nav button component for header buttons * remove tooltip button stylesheet * make header a functional component and implement nav buttons * remove animation on home button * remove unneeded style sheets and class names from action button components * use interface for tooltip text * remove unused import * delete unused stylesheet * move tooltip placement type to constants * default nav buttons to action styling and trim selectors * provide default disabled tooltip text and simplify conditional checks commit f527adb Author: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Date: Thu Mar 28 11:16:17 2024 -0700 Bump webpack-dev-middleware from 5.3.3 to 5.3.4 (#481) Bumps [webpack-dev-middleware](https://github.com/webpack/webpack-dev-middleware) from 5.3.3 to 5.3.4. - [Release notes](https://github.com/webpack/webpack-dev-middleware/releases) - [Changelog](https://github.com/webpack/webpack-dev-middleware/blob/v5.3.4/CHANGELOG.md) - [Commits](webpack/webpack-dev-middleware@v5.3.3...v5.3.4) --- updated-dependencies: - dependency-name: webpack-dev-middleware dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

github-actions · 2024-04-02T23:34:08Z

Coverage report

St.^❔	Category	Percentage	Covered / Total
🟡	Statements	73.43% (+0.16% 🔼)	608/828
🟡	Branches	69.92% (+1.54% 🔼)	86/123
🔴	Functions	40.61% (+0.3% 🔼)	80/197
🟡	Lines	71.92% (+0.19% 🔼)	543/755

Test suite run success

102 tests passing in 7 suites.

Report generated by 🧪jest coverage report action from e93b7a9

… into feature/record-movies

meganrm · 2024-04-16T22:05:55Z

src/components/RecordMoviesComponent/index.tsx

+    const stopRecordingIcon = isHovering
+        ? "stop-record-icon"
+        : activeRecordingIcon;
+
+    const getIcon = () => {
+        return isRecording ? stopRecordingIcon : startRecordingIcon;
+    };


nit: but these two things seem really similar, but one is just a constant and one is a function? unless there is a reason I would pick one setup and go with it for both

meganrm · 2024-04-16T22:26:41Z

src/containers/ViewerPanel/index.tsx

@@ -99,6 +103,7 @@ interface ViewerPanelState {
    particleTypeIds: string[];
    height: number;
    width: number;
+    movieURL: string;


since this is stored in local state and only sent down to one component, why not just have it stored by that one component?

The onRecordedMovie handler receives the blob of data from the viewer, so we need to do something with that blob in the component where the callback is defined and passed to the viewer.

We could pass the blob to the lower component as a prop and define the URL there, but I figured that passing the blob around, rather than just a URL, could potentially have performance implications?

oh never mind, I see, you needed it for the onRecordMovie prop

meganrm · 2024-04-16T22:27:21Z

src/containers/ViewerPanel/index.tsx

+        });
+    };
+
+    private getMovieTitle = () => {


this seems like it could be a selector?

Yeah, good call

meganrm · 2024-04-16T22:32:35Z

src/components/ViewportButton/index.tsx

+    const getTooltip = () =>
+        !disabled || tooltipWhenDisabled ? tooltipText : "";
+
+    const getClickHandler = () => (!disabled ? clickHandler : undefined);


I'm surprised typescript isn't upset about having onClick={undefined}.
Just a suggestion:

Suggested change

const getClickHandler = () => (!disabled ? clickHandler : undefined);

onClick = () => {

if (disabled) {return} else {clickHander()}

}

onClick={onClick}

the only difference being that it's now a function that returns nothing rather than setting it to undefined. but it's fine either way.

I'm fine either way, happy to make a change if its less confusing.

I think TS doesn't mind because the antd Button component onClick prop is optional, so undefined is an acceptable thing to return from that function.

Returning undefined seems fine to me, but I might consider making these just plain constants.

meganrm

you have a linting error with a now un-needed import, otherwise looks good

frasercl

Looks good!

frasercl · 2024-04-23T20:16:13Z

src/components/ViewportButton/index.tsx

+    const getTooltip = () =>
+        !disabled || tooltipWhenDisabled ? tooltipText : "";
+
+    const getClickHandler = () => (!disabled ? clickHandler : undefined);


Returning undefined seems fine to me, but I might consider making these just plain constants.

interim17 added 9 commits March 25, 2024 11:29

use custom modal wrapper for more layout and styling

9b8a6f4

remove duplicate and unused colors

c60bf01

remove more duplicated colors

d02e834

remove comments

a97648c

remove unused import from starcheckbox

3ecfbaa

simplify time input css selector

a0c4350

implement record movies ui

5e16dad

only store video url in state, get duration in record movies compennt…

ab776ca

… and title in download handler

move download function into record movie component

20698db

interim17 commented Mar 26, 2024

View reviewed changes

getMovieTitle can be a private method

d08465c

interim17 changed the title ~~Feature/record movies~~ Feature/record movie UI Mar 26, 2024

interim17 added blocked and removed blocked labels Mar 27, 2024

Base automatically changed from fix/update-custom-modal to main April 2, 2024 16:12

interim17 added 2 commits April 2, 2024 16:23

Merge branch 'main' into feature/record-movies

c27a32a

interim17 added 3 commits April 3, 2024 09:43

remove unused variables

dec6652

organization and clarifying comments

6ecb3c6

Merge branch 'main' of https://github.com/simularium/simularium-website…

5146d1c

… into feature/record-movies

interim17 marked this pull request as ready for review April 16, 2024 20:43

interim17 requested a review from a team as a code owner April 16, 2024 20:43

interim17 requested review from meganrm and frasercl and removed request for a team April 16, 2024 20:43

meganrm reviewed Apr 16, 2024

View reviewed changes

interim17 added 6 commits April 16, 2024 15:50

use single function to get icon in record movies component

cc8f14d

use selector to get movie title

4497ddd

rename cleanup movie state to resetAfterMovieRecording

b81f2ac

move useFormattedDuration hook into its own file

73db0f2

remove unsed comment

f48f6ac

add test for getMovieTitle

ab01a98

meganrm approved these changes Apr 18, 2024

View reviewed changes

remove unused import

e93b7a9

meganrm mentioned this pull request Apr 23, 2024

point at octopus cluster #500

Merged

frasercl approved these changes Apr 23, 2024

View reviewed changes

interim17 merged commit a9280a9 into main Apr 23, 2024
7 checks passed

interim17 deleted the feature/record-movies branch April 23, 2024 23:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/record movie UI #483

Feature/record movie UI #483

interim17 commented Mar 26, 2024 •

edited

Loading

interim17 Mar 26, 2024

interim17 Mar 26, 2024 •

edited

Loading

interim17 Mar 26, 2024

meganrm Apr 16, 2024

interim17 Mar 26, 2024

github-actions bot commented Apr 2, 2024 •

edited

Loading

meganrm Apr 16, 2024

meganrm Apr 16, 2024

interim17 Apr 16, 2024

meganrm Apr 16, 2024

meganrm Apr 16, 2024

interim17 Apr 16, 2024

meganrm Apr 16, 2024

interim17 Apr 16, 2024

frasercl Apr 23, 2024

meganrm left a comment

frasercl left a comment

frasercl Apr 23, 2024

-    const getClickHandler = () => (!disabled ? clickHandler : undefined);
+    onClick = () => {
+       if (disabled) {return} else {clickHander()}
+    }
+    onClick={onClick}

Feature/record movie UI #483

Feature/record movie UI #483

Conversation

interim17 commented Mar 26, 2024 • edited Loading

Time estimate or Size

Problem

Solution

Steps to Verify:

Screenshots (optional):

Choose a reason for hiding this comment

interim17 Mar 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Apr 2, 2024 • edited Loading

Coverage report

Test suite run success

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

meganrm left a comment

Choose a reason for hiding this comment

frasercl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

interim17 commented Mar 26, 2024 •

edited

Loading

interim17 Mar 26, 2024 •

edited

Loading

github-actions bot commented Apr 2, 2024 •

edited

Loading