Redesign #1079

Arnei · 2023-06-12T06:42:14Z

Resolves #1076.

Currently a draft due to still being worked on.

Changes the appearance of the editor to bring it more in line with Opencast Studio and Tobira. Focuses on visual changes, functional changes should be minimal.

The focus of the redesign lies in the color scheme for the light mode. Furthermore, elements should be more distinct and intuitive through the use of additional backgrounds and rearrangement.
Does not aim to make major changes to:

Thumbnail View
Subtitle View
High Contrast modes

What this will aspire to look like

Incidentally resolves #897.
Incidentally resolves #898.
Incidentally resolves #65.

Graciously sponsored by the ETH.

Styles header according to redesgin

Changes the main menu text to blue and bold. Tries to keep text black where suggested.

github-actions · 2023-06-12T06:45:30Z

This pull request is deployed at test.editor.opencast.org/1079/2023-06-12_06-42-42/ .
It might take a few minutes for it to become available.

Replaces the fontawesome packages with the react-icons package. React-icons also includes fontawesome icons, but also has feather icons which we want to predominantly use in the redesign. Attempts to replace fontawesome icons with appropriate feather icons. Also grabs a subtitle icon from material-ui- icons.

github-actions · 2023-06-13T11:49:58Z

This pull request has conflicts ☹
Please resolve those so we can review the pull request.
Thanks.

github-actions · 2023-06-13T11:53:17Z

This pull request is deployed at test.editor.opencast.org/1079/2023-06-13_11-50-11/ .
It might take a few minutes for it to become available.

github-actions · 2023-06-13T11:59:26Z

This pull request is deployed at test.editor.opencast.org/1079/2023-06-13_11-56-34/ .
It might take a few minutes for it to become available.

Splits `Video.tsx` into three new files. `Cutting.tsx` now is in charge of the cutting view (before this was awkwardly split between MainContent.tsx and Video.tsx). VideoPlayers and VideoControls now each have their own files so they can be arranged flexibly.

Works on the appearance of the cutting view, mostly concerned with boxes and boxshadows.

github-actions · 2023-06-14T14:59:32Z

This pull request is deployed at test.editor.opencast.org/1079/2023-06-14_14-55-48/ .
It might take a few minutes for it to become available.

Attempts to fit the timeline to the colors of the redesign. Uses a hacky filter.

Reshapes the scrubber to fit the basic idea of the redesign. For a better fit somebody with artistic skills would have to create an SVG.

Now centered and boxShado-less!

Also aim at improving the general layout of the cutting view again.

The videos now have rounded edges. Finally.

github-actions · 2023-06-15T14:21:19Z

This pull request is deployed at test.editor.opencast.org/1079/2023-06-15_14-18-09/ .
It might take a few minutes for it to become available.

boxshadow did not conform to the borderradius

General Metadata Layout. Also some color changes where easy to make. More color changes to come.

Fixes an invalid overflow value in MainContent. Would previously only have scrollbars for the whole page, which would leave the main menu looking awkwardly cut off. Now scrolls properly.

Changes the design for the track selection

Arnei · 2023-08-31T08:25:55Z

If people are happy with this PR in general and there are no more requests for large scale changes, it would be great if we could get it approved and merged. Since this PR touches most parts of the editor, it also blocks a lot of other PRs. Getting this PR merged would free those up. And getting this PR merged does not mean the redesign has to be done and over with, we can still make follow-up PRs for smaller scale changes.

github-actions · 2023-08-31T10:26:16Z

This pull request has conflicts ☹
Please resolve those so we can review the pull request.
Thanks.

github-actions · 2023-08-31T10:38:31Z

This pull request is deployed at test.editor.opencast.org/1079/2023-08-31_10-35-29/ .
It might take a few minutes for it to become available.

github-actions · 2023-08-31T10:54:23Z

This pull request is deployed at test.editor.opencast.org/1079/2023-08-31_10-51-23/ .
It might take a few minutes for it to become available.

lkiesow · 2023-08-31T21:05:28Z

Unfortunately, it seems the translation library we use (react-18next) does not support an auto option out of the box. We certainly could work around that and make our own auto mode somehow, but it seems to me like that would take more time than I am willing to spent on it in this PR. Best left for another time.

If I remember correctly, it does support that to some extent: It already selects the language automatically, if none is stored in the local storage. So maybe just store to auto-mode separately and make sure to remove the stored i18m language before loading i18m.

But I'm absolutely fine with moving that to a separate patch.

github-actions · 2023-09-01T13:51:46Z

This pull request has conflicts ☹
Please resolve those so we can review the pull request.
Thanks.

github-actions · 2023-09-04T13:43:34Z

This pull request is deployed at test.editor.opencast.org/1079/2023-09-04_13-39-17/ .
It might take a few minutes for it to become available.

owi92

Ok so I've looked over the code changes and left some comments, but tbh these are just personal preferences and I wouldn't say that you actually need to address any of these.

I also took another look at the deployment and noticed a few small things that should probably get fixed:

High contrast mode:
- (dark only) The menus for language and appearance should have a visible border. Since they have the same background color as the general background, they look a little weird without that:
- (dark and light) The subtitle picker barely has any contrast on hover:
Metadata:
- the translation for portugese is missing:
Subtitle editor:
- there's a cut-off button:
- I think the timeline overview could use some margin on the bottom. Without, it's kind of hard to notice:

src/main/Cutting.tsx

+        <Timeline
+          timelineHeight={260}
+          selectIsPlaying={selectIsPlaying}
+          selectCurrentlyAt={selectCurrentlyAt}
+          setIsPlaying={setIsPlaying}
+          setCurrentlyAt={setCurrentlyAt}
+          setClickTriggered={setClickTriggered}
+        />
+        <CuttingActions />
+        <VideoControls
+          selectCurrentlyAt={selectCurrentlyAt}
+          selectIsPlaying={selectIsPlaying}
+          selectIsPlayPreview={selectIsPlayPreview}
+          setIsPlaying={setIsPlaying}
+          setIsPlayPreview={setIsPlayPreview}
+        />


owi92 · 2023-09-12T10:34:29Z

src/main/CuttingActions.tsx

+        <CuttingActionsButton Icon={LuScissors}
+          actionName={t("cuttingActions.cut-button")} actionHandler={dispatchAction} action={cut}
+          tooltip={t('cuttingActions.cut-tooltip', { hotkeyName: (cuttingKeyMap[handlers.cut.name] as KeyMapOptions).sequence })}
+          ariaLabelText={t('cuttingActions.cut-tooltip-aria', { hotkeyName: (cuttingKeyMap[handlers.cut.name] as KeyMapOptions).sequence })}


I would generally list one prop per line for every component, if they don't fit in one line. I think that improves readability. Something like

<CuttingActionsButton Icon={LuScissors} actionName={t("cuttingActions.cut-button")} actionHandler={dispatchAction} action={cut} tooltip={t('cuttingActions.cut-tooltip', { hotkeyName: (cuttingKeyMap[handlers.cut.name] as KeyMapOptions).sequence })} ariaLabelText={t('cuttingActions.cut-tooltip-aria', { hotkeyName: (cuttingKeyMap[handlers.cut.name] as KeyMapOptions).sequence })} />

Though as with my other comment, I see that this is done in many other places as well. So I think if that's how you like doing it, there is nothing wrong with it.

owi92 · 2023-09-12T10:52:12Z

src/main/SubtitleVideoArea.tsx

+            first={true}
+            last={true}


You can pass true boolean props without the ={true} and spread some of the other props like I suggested in one of my other comments.

owi92 · 2023-09-12T11:21:23Z

src/main/SubtitleListEditor.tsx

+      marginTop: (scheme === 'dark-high-contrast' || scheme === 'light-high-contrast' ? '3%' : '0%'),
+      marginBottom: (scheme === 'dark-high-contrast' || scheme === 'light-high-contrast' ? '3%' : '0%'),


You couuuld use the shorthand for these:

margin: `${(scheme === 'dark-high-contrast' || scheme === 'light-high-contrast' ? '3%' : '0%')} 0`,

and also above that (not strictly part of this review), from line 310 to 319, you could bunch the hover and focus style together, like:

"&:hover, &:focus-within": { "& .functionButtonAreaStyle": { visibility: "visible", } },

owi92 · 2023-09-12T11:42:11Z

src/main/Timeline.tsx

-    height: timelineHeight - 20 + 'px',   // CHECK IF     height: '100%',
-    paddingTop: '10px',
+    height: timelineHeight + 'px',   // CHECK IF     height: '100%',
+    // paddingTop: '10px',


Maybe just remove this commented-out padding? Same for line 348. Though maybe there's a reason these are commented out and not removed.

owi92 · 2023-09-12T12:43:05Z

src/main/VideoPlayers.tsx

+        ref.current.seekTo(currentlyAt, "seconds")
+        dispatch(setClickTriggered(false))
+      }
+      if (!isAspectRatioUpdated && ready) {     //     if (!isAspectRatioUpdated && ref.current && ready) {


You can probably also remove that comment on the right.

owi92 · 2023-09-12T13:01:40Z

src/main/VideoPlayers.tsx

+  // return (
+  //   <div title="Video Player">
+  //     <video width="320" height="240" controls ref={vidRef}>
+  //     <source src="https://media.geeksforgeeks.org/wp-content/uploads/20190616234019/Canvas.move_.mp4" type="video/mp4" />
+  //     Your browser does not support the video tag.
+  //     </video>
+  //   </div>
+  // );


Another one, though as with the others, maybe you have a reason for keeping this around.

owi92 · 2023-09-12T13:09:15Z

src/main/VideoPlayers.tsx

+    return (
+      <>
+        {render()}
+      </>
+    );


Nothing wrong with this, but you could do sth like

return errorState ? <div css={errorBoxStyle} role="alert"> <span>{t("video.loadError-text")} </span> </div> : <ReactPlayer (...) />

In high contrast mode, hovering the button containing the subtitle dropdown would color all elements, making it unreadable. Should now be more readable

Language key for portugese changed in Opencast, so we need to change it in our translation files as well.

Arnei · 2023-09-12T15:48:13Z

Thanks for the review Ole

(dark only) The menus for language and appearance should have a visible border. Since they have the same background color as the general background, they look a little weird without that:

Agreed, will look into that. Will likely have to fix it in appkit though. Could this also be done in a follow-up PR or would you consider this a blocker?

(dark and light) The subtitle picker barely has any contrast on hover:

Should be better now.

the translation for portugese is missing:

Should be fixed now (for english)

Subtitle Editor and Code style

While these are valid concerns, most of these already exists prior to this PR. Thus, it should not be the responsibility of this PR to fix them, let alone discuss which code styles we want to pursue. We can address these topics in separate issues.

github-actions · 2023-09-12T15:49:05Z

This pull request is deployed at test.editor.opencast.org/1079/2023-09-12_15-46-17/ .
It might take a few minutes for it to become available.

gregorydlogan · 2023-09-12T16:52:25Z

While I haven't looked at the code, the UI itself looks good to me.

owi92 · 2023-09-12T21:03:11Z

Agreed, will look into that. Will likely have to fix it in appkit though. Could this also be done in a follow-up PR or would you consider this a blocker?

Fair point, and I don't consider this a blocker.

(dark and light) The subtitle picker barely has any contrast on hover:

Should be better now.

The contrast is better, though I'd probably prefer sth like this on hover:
(But I would also say this can be moved to another issue and doesn't need to be done in this PR, if at all)

While these are valid concerns, most of these already exists prior to this PR. Thus, it should not be the responsibility of this PR to fix them, let alone discuss which code styles we want to pursue. We can address these topics in separate issues.

Yeah I figured as much, especially regarding the code styles.
So I'd say this can be merged.

oas777 · 2023-09-13T12:56:11Z

I know I'm late to the game, but I'm still struggeling with the diversity of canvasses, cf. #1106.

Title ("Presenter" or "Presentation) or not?

vs.

vs.

Functions/symbols below or to the right?

vs.

And, as a consequence, size, resolution, and appearance are very diverse over only three "elements" ("Cutting", "Tracks", "Thumbnail")

oas777 · 2023-09-13T12:57:41Z

PS: I'm in favor of titles in every canvas and symbols to the right, a vertical alignment as a consequence.

Arnei · 2023-09-13T13:35:17Z

Created a new issue for the symbols below/to the right in #1157 (trying to move discussions out of this really long thread).

Arnei added 3 commits June 9, 2023 15:12

Redesign - Header Style

823cb9b

Styles header according to redesgin

Redesign - Main Menu Text

caecebc

Changes the main menu text to blue and bold. Tries to keep text black where suggested.

Redesign - Main Menu Button Shape

9496b55

Arnei added type:enhancement New feature or request type:visual-clarity A part of the UI is not visually readable labels Jun 12, 2023

github-actions bot added the status:conflicts Conflicts with another pull request or issue label Jun 13, 2023

Redesign: Change background color

ebcf6b7

Arnei added 4 commits June 14, 2023 12:02

Merge remote-tracking branch 'upstream/main' into redesign

3a1b4ea

Merge remote-tracking branch 'upstream/main' into redesign

df91647

Redesign: General cutting view appearance

5ff9461

Works on the appearance of the cutting view, mostly concerned with boxes and boxshadows.

github-actions bot removed the status:conflicts Conflicts with another pull request or issue label Jun 14, 2023

Arnei added 5 commits June 15, 2023 10:38

Redesign: Timeline color

52638d8

Attempts to fit the timeline to the colors of the redesign. Uses a hacky filter.

Redesign: Scrubber basic idea

bd35e07

Reshapes the scrubber to fit the basic idea of the redesign. For a better fit somebody with artistic skills would have to create an SVG.

Redesign: Cutting Buttons

fcefd54

Now centered and boxShado-less!

Redesign: VideoControls Layout

9098782

Also aim at improving the general layout of the cutting view again.

Redesign: Rounded edges for videos

1d301ab

The videos now have rounded edges. Finally.

Arnei added 6 commits June 15, 2023 16:53

Redesign: Fix boxshadow on videoPlayers

b193c39

boxshadow did not conform to the borderradius

Redesign: Fix vertical scrollbar appearing

bb20602

Redesign: Metadata Layout

0883783

General Metadata Layout. Also some color changes where easy to make. More color changes to come.

Redesign: Metadata colors

2d84a8f

Fix no scrollbars in MainContent

4097ca1

Fixes an invalid overflow value in MainContent. Would previously only have scrollbars for the whole page, which would leave the main menu looking awkwardly cut off. Now scrolls properly.

Redesign: Track Selection

a99b16d

Changes the design for the track selection

Arnei mentioned this pull request Aug 29, 2023

Add "auto" option to language selector #1137

Open

lkiesow mentioned this pull request Aug 29, 2023

Create a continuously updating test system elan-ev/studip-opencast-plugin#777

Closed

github-actions bot added the status:conflicts Conflicts with another pull request or issue label Aug 31, 2023

Merge remote-tracking branch 'upstream/main' into redesign

ec68a04

Merge remote-tracking branch 'upstream/main' into redesign

c1eb1ca

github-actions bot removed the status:conflicts Conflicts with another pull request or issue label Aug 31, 2023

github-actions bot added the status:conflicts Conflicts with another pull request or issue label Sep 1, 2023

Merge remote-tracking branch 'upstream/main' into redesign

9a70d50

github-actions bot removed the status:conflicts Conflicts with another pull request or issue label Sep 4, 2023

owi92 suggested changes Sep 12, 2023

View reviewed changes

Arnei added 2 commits September 12, 2023 17:40

High Contrast: Fix subtitle select hover color

ad5f1d5

In high contrast mode, hovering the button containing the subtitle dropdown would color all elements, making it unreadable. Should now be more readable

Fix language key for portugese

52b0ac7

Language key for portugese changed in Opencast, so we need to change it in our translation files as well.

owi92 approved these changes Sep 12, 2023

View reviewed changes

Arnei merged commit 184e15b into opencast:main Sep 13, 2023
6 checks passed

Arnei mentioned this pull request Sep 13, 2023

Button Placement in Track and Thumbnail #1157

Closed

LukasKalbertodt mentioned this pull request Oct 4, 2023

Add icon guidelines opencast/appkit#2

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Redesign #1079

Redesign #1079

Arnei commented Jun 12, 2023 •

edited

Loading

github-actions bot commented Jun 12, 2023

github-actions bot commented Jun 13, 2023

github-actions bot commented Jun 13, 2023

github-actions bot commented Jun 13, 2023

github-actions bot commented Jun 14, 2023

github-actions bot commented Jun 15, 2023

Arnei commented Aug 31, 2023

github-actions bot commented Aug 31, 2023

github-actions bot commented Aug 31, 2023

github-actions bot commented Aug 31, 2023

lkiesow commented Aug 31, 2023

github-actions bot commented Sep 1, 2023

github-actions bot commented Sep 4, 2023

owi92 left a comment

This comment was marked as resolved.

owi92 Sep 12, 2023

owi92 Sep 12, 2023

owi92 Sep 12, 2023

owi92 Sep 12, 2023

owi92 Sep 12, 2023

owi92 Sep 12, 2023

owi92 Sep 12, 2023

Arnei commented Sep 12, 2023

github-actions bot commented Sep 12, 2023

gregorydlogan commented Sep 12, 2023

owi92 commented Sep 12, 2023

oas777 commented Sep 13, 2023

oas777 commented Sep 13, 2023

Arnei commented Sep 13, 2023

		marginTop: (scheme === 'dark-high-contrast' \|\| scheme === 'light-high-contrast' ? '3%' : '0%'),
		marginBottom: (scheme === 'dark-high-contrast' \|\| scheme === 'light-high-contrast' ? '3%' : '0%'),

Redesign #1079

Redesign #1079

Conversation

Arnei commented Jun 12, 2023 • edited Loading

github-actions bot commented Jun 12, 2023

github-actions bot commented Jun 13, 2023

github-actions bot commented Jun 13, 2023

github-actions bot commented Jun 13, 2023

github-actions bot commented Jun 14, 2023

github-actions bot commented Jun 15, 2023

Arnei commented Aug 31, 2023

github-actions bot commented Aug 31, 2023

github-actions bot commented Aug 31, 2023

github-actions bot commented Aug 31, 2023

lkiesow commented Aug 31, 2023

github-actions bot commented Sep 1, 2023

github-actions bot commented Sep 4, 2023

owi92 left a comment

Choose a reason for hiding this comment

This comment was marked as resolved.

owi92 Sep 12, 2023

Choose a reason for hiding this comment

owi92 Sep 12, 2023

Choose a reason for hiding this comment

owi92 Sep 12, 2023

Choose a reason for hiding this comment

owi92 Sep 12, 2023

Choose a reason for hiding this comment

owi92 Sep 12, 2023

Choose a reason for hiding this comment

owi92 Sep 12, 2023

Choose a reason for hiding this comment

owi92 Sep 12, 2023

Choose a reason for hiding this comment

Arnei commented Sep 12, 2023

github-actions bot commented Sep 12, 2023

gregorydlogan commented Sep 12, 2023

owi92 commented Sep 12, 2023

oas777 commented Sep 13, 2023

oas777 commented Sep 13, 2023

Arnei commented Sep 13, 2023

Arnei commented Jun 12, 2023 •

edited

Loading