elan-ev / opencast-studio

Web-based recording studio for Opencast
https://studio.opencast.org
MIT License
50 stars 47 forks source link

Remove warning triangle and center error message for video select errors #1113

Closed LukasKalbertodt closed 1 year ago

LukasKalbertodt commented 1 year ago

Fixes #1103

This also fixes this error case for narrow screens where the error message previously overflowed the container, sometimes even the viewport.

Test deployment: https://test.studio.opencast.org/2023-09-26_12-46-54-pr1113-6312834670/

lkiesow commented 1 year ago

I think the (missing) padding around the warning box looks pretty weird.

Screenshot from 2023-09-15 13-16-50

Simply adding more padding makes this look a lot better: Screenshot from 2023-09-15 13-17-35

If you really want it this small, then make the whole box red:

Screenshot from 2023-09-15 13-16-50

LukasKalbertodt commented 1 year ago

Thanks for comment, I indeed completely missed the weird horizontal margin on wider screens. I agree that looks weird. I removed that now so that the margin around the red box is always the same on all sides. Your suggestion of adding lots more margin is difficult to implement due to the auto-sizing algorithm that's involved here. And that's especially tricky with two video inputs.

I personally prefer having the white box around it still. I think the contrast of the red background to the grey background is not too great. What do you think? Please keep in mind that this is a rather rare situation where legibility is the most important aspect I'd say. Most normal users will never see this error.

Also CC @narickmann @oas777 you commented on the issue, but let's continue the discussion here.

image

image

image

oas777 commented 1 year ago

I'm usually a friend of padding/canvasses, but here I would prefer to remove the white box. But I don't have strong feelings regarding this...

oas777 commented 1 year ago

PS: Revisiting this a couple of hours later, my feelings are getting stronger for removing the white box. @narickmann?

LukasKalbertodt commented 1 year ago

I removed the margin now. With also the box shadow removed in those cases, i would agree that it looks better. Test for yourself.

image image

oas777 commented 1 year ago

I think it does, obviously... but from my high uniformity horse I cannot oversee the fact that the two boxes are different in height...

grafik

PS: Why is the text different with my settings?

LukasKalbertodt commented 1 year ago

@oas777 The boxes contain different texts, so that's why their height is different. One could make them a fixed height but that's not ideal, especially since in different languages the text could take up less or more space. Also, there is one error message (when you stop a running screen share for example) that is very short and would look weird in a large box. Again remember that this is something people will very rarely see.

Not sure what your PS question is. Surely you don't mean that you have German text because your Studio was set to German?


@lkiesow are your concerns addressed by the latest push?

github-actions[bot] commented 1 year ago

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

oas777 commented 1 year ago

From a uniformity perspective, the length of the text is relevant when complaining about the height of the boxes, but maybe I should stop before asking for uniform error messages with the same text lengths...

oas777 commented 1 year ago

PS: My error message is "Cannot access display", the one you shared starts with "Video stream unexpectedly...", both are for content, I presume. What did you do to trigger yours?

LukasKalbertodt commented 1 year ago

What did you do to trigger yours?

I stopped an active screen share via my browser.


It sounds like this could be merged then. Will just ask someone to review the code.

oas777 commented 1 year ago

What did you do to trigger yours?

I stopped an active screen share via my browser.

I was able to reproduce this, so...

It sounds like this could be merged then. Will just ask someone to review the code.

... yes.

github-actions[bot] commented 1 year ago

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

LukasKalbertodt commented 1 year ago

There is still a warning triangle if you cause an error during recording (e.g. by revoking permissions). Is that intended?

I had hoped nobody would notice that Good point, thanks! I fixed that now. Now unexpected stream end in the recording step looks like in the video select step.

But your comment also made me notice another bug that I also fixed. Should be good to go now.