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.

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

Thumbnail Editor #760

Merged
merged 27 commits into from
Oct 6, 2022
Merged

Thumbnail Editor #760

merged 27 commits into from
Oct 6, 2022

Conversation

Arnei
Copy link
Member

@Arnei Arnei commented Aug 17, 2022

Adds a new view which allows users to create thumbnails for their events.

Allows users to:

  • Generate a thumbnail from a video at a selected point in time.
  • Upload a thumbnail.
  • Set the same thumbnail for all videos.
  • Discard any changes they made.

Has a "simple" and a "professional" mode.

  • Simple: For when you only care about one primary thumbnail.
  • Professional: When you want to be able to change all the thumbnails in the subflavor.

Requires Opencast backend PR to function: opencast/opencast#4086

@github-actions
Copy link

This pull request is deployed at test.editor.opencast.org/760/2022-08-17_07-25-21/ .
It might take a few minutes for it to become available.

Copy link
Contributor

@geichelberger geichelberger left a comment

Choose a reason for hiding this comment

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

The view seems a little bit messy 🇩🇪unruhig. Messy is the wrong word.

  • The placeholder for the thumbnail could have a fixed ratio, like 16:9.
  • Maybe deactivate the buttons that are not usable if a thumbnail is not defined

grafik

src/main/Video.tsx Show resolved Hide resolved
@github-actions
Copy link

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

@github-actions
Copy link

This pull request is deployed at test.editor.opencast.org/760/2022-08-19_06-23-40/ .
It might take a few minutes for it to become available.

@narickmann
Copy link
Contributor

I agree with @geichelberger

The placeholder for the thumbnail could have a fixed ratio, like 16:9.

The box could have more width or at least some padding left and right. The text is right at the border and looks a bit squeezed.


Three other things I noticed:

First one: Maybe you could make the buttons "Generate" and "Use for other thumbnails" the same size? (See picture, I adjusted the size for these two buttons above). When the buttons have the same size, they maybe look more uniform/consistent?

grafik

Second thing: When I generate a thumbnail, the thumbnail and the buttons are very far apart. If you use a zooming-software, you (maybe) don't see that they belong together. (Something I learned about accessibility :D )

And the third thing: The box is right at the bottom of the page. I think some margin would fix this.

grafik

@github-actions
Copy link

This pull request is deployed at test.editor.opencast.org/760/2022-08-19_08-09-00/ .
It might take a few minutes for it to become available.

@Arnei
Copy link
Member Author

Arnei commented Aug 19, 2022

Thanks for your feedback. That'll teach me to only test things in Chrome and think "Should I test other browers? Naah, it'll be fine".

@geichelberger
Copy link
Contributor

Firefox seems to be picky :)

Firefox

grafik

Chrome

grafik

@geichelberger
Copy link
Contributor

And it ignores the ratio if no thumbnail is set, which is working in Chrome.
grafik

@narickmann
Copy link
Contributor

narickmann commented Aug 19, 2022

True. I know what you mean. Most of the time I only test in Firefox (as for my review). In Chrome it doesn't take the whole width, when you generated a thumbnail. This time I tested in Firefox and Chrome.

The buttons look way better now and the view in Chrome fits very well

There are more things I noticed: (edited)

  • There is no support for darkmode -> Chrome and Firefox
  • The buttons do not have the same width and alignment after generating thumbnails (didn't noticed it before, because I only generated one thumbnail) -> only in Firefox
  • I know german words can be very long, the text of the navigation-button hits the edges. The word "Miniaturbilder" maybe is the best solution so far. I've thought about alternative words, but couldn't find a shorter word that fits. "Vorschaubilder" is longer and also doesn't fit in. -> Chrome and Firefox
  • The box for the thumbnail is still small/slim -> only Firefox

Btw: I test with Fedora 35 and Firefox 103.0 and Chrome 104.0.

This is how it looks with Firefox:

grafik

grafik

@narickmann
Copy link
Contributor

Oh no, you were faster than me. Let me check and correct my review.

@github-actions
Copy link

This pull request is deployed at test.editor.opencast.org/760/2022-08-19_09-05-04/ .
It might take a few minutes for it to become available.

@Arnei
Copy link
Member Author

Arnei commented Aug 19, 2022

There is no support for darkmode -> Chrome and Firefox

Could you be more specific? The page looks pretty dark in dark mode to me.

@narickmann
Copy link
Contributor

I updated my last review. It looks way better now in Firefox.

@ziegenberg
Copy link
Member

I would also like to have icons for the buttons, but I could not come up with fitting icons for every button and did not want to cause more confusion by only having icons on like half the buttons.

How about the following icons?

Button FA-Name icon
Generate fa-camera camera-solid
Generate (alternative) fa-brush brush-solid
Upload fa-upload upload-solid
Synchronize fa-copy copy-solid
Discard fa-trash trash-solid
Discard (alternative) fa-circle-xmark circle-xmark-solid

@Arnei
Copy link
Member Author

Arnei commented Sep 5, 2022

I agree with your assessment that the thumbnail preview ought to be bigger and the buttons can be smaller, so I tried to implement that. Although I tried to refrain from using aspect-ratio, from my experience it behaves too differently in different browsers to be reliable.

@github-actions
Copy link

github-actions bot commented Sep 5, 2022

This pull request is deployed at test.editor.opencast.org/760/2022-09-05_13-51-46/ .
It might take a few minutes for it to become available.

@Arnei
Copy link
Member Author

Arnei commented Sep 5, 2022

I personally don't mind the length of "Use for other thumbnails". I always like to try and make the UI more dynamic before resorting to shorten the text, as it ends up helping out with translations (especially the germans somehow always end up with words that are way too long). I also find "Use for other thumbnails" easier to understand than shorter alternatives.

But I can also see the case for a shorter version. Ultimately I don't care too much about the name of the buttons, maybe everyone could vote for their favourite version on crowdin?

@github-actions
Copy link

github-actions bot commented Sep 5, 2022

This pull request is deployed at test.editor.opencast.org/760/2022-09-05_14-14-44/ .
It might take a few minutes for it to become available.

@ziegenberg
Copy link
Member

I very much like it now!
Though one more small thing. There is a size difference between the placeholder (355x200) and an actual thumbnail (498x280).

Screen Shot 2022-09-05 at 17 33 41

Screen Shot 2022-09-05 at 17 33 44

@github-actions
Copy link

github-actions bot commented Sep 6, 2022

This pull request is deployed at test.editor.opencast.org/760/2022-09-06_07-03-20/ .
It might take a few minutes for it to become available.

@github-actions
Copy link

github-actions bot commented Sep 6, 2022

This pull request is deployed at test.editor.opencast.org/760/2022-09-06_07-53-21/ .
It might take a few minutes for it to become available.

@ziegenberg
Copy link
Member

Another thing I found. The buttons increase their size when hovering over them. That's OK. They also increase their size when clicking. That's also OK. But they stay in the increased size, after clicking and when no longer hovering over them. I have to click somewhere else for them to be restored to their original size.

@ziegenberg
Copy link
Member

The accessibility check only reports one issue. For the upload buttons an invisible <input /> is created. This should probably have aria-hidden="true" set to hide this non-interactive content from the accessibility API.

Screenshot from 2022-09-06 12-27-50

@github-actions
Copy link

github-actions bot commented Sep 6, 2022

This pull request is deployed at test.editor.opencast.org/760/2022-09-06_13-24-32/ .
It might take a few minutes for it to become available.

@github-actions
Copy link

github-actions bot commented Sep 6, 2022

This pull request is deployed at test.editor.opencast.org/760/2022-09-06_13-32-55/ .
It might take a few minutes for it to become available.

Copy link
Member

@ziegenberg ziegenberg left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@github-actions
Copy link

github-actions bot commented Sep 6, 2022

This pull request is deployed at test.editor.opencast.org/760/2022-09-06_14-46-20/ .
It might take a few minutes for it to become available.

Copy link
Contributor

@narickmann narickmann left a comment

Choose a reason for hiding this comment

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

Looks good. Great work.

I am missing german translations, but this should not stop this PR from being merged.

@geichelberger
Copy link
Contributor

geichelberger commented Sep 14, 2022

The approach of creating the thumbnail client side has a considerable disadvantage. The thumbnail quality depends on the video editor preview, which usually has a lower quality/resolution. I think that's not a show-stopper for this PR, but there is room for improvement.

@ziegenberg
Copy link
Member

ziegenberg commented Oct 5, 2022

The approach of creating the thumbnail client side has a considerable disadvantage. The thumbnail quality depends on the video editor preview, which usually has a lower quality/resolution. I think that's not a show-stopper for this PR, but there is room for improvement.

I created a new issue for this comment: #814

@ziegenberg ziegenberg merged commit 113b44d into opencast:main Oct 6, 2022
@ziegenberg ziegenberg mentioned this pull request Oct 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type:feature A new feature or feature request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants