Thumbnail make boxes same size #1210
Conversation
Currently in the thumbnail view, the videos are the same size as the thumbnails. However, the outlines surrounding them are different, as the outline around the thumbnails also contains buttons. This enlarges the videos to make the outlines have the same width.
The timeline in the thumbnail view did not stretch to the full width of the available screenspace in some cases (e.g. when there is only one video). This fixes that.
|
This pull request is deployed at test.editor.opencast.org/1210/2023-11-07_11-05-10/ . |
| @@ -37,7 +37,7 @@ const VideoPlayers: React.FC<{refs: any, widthInPercent?: number}> = ({refs, wid | |||
| borderRadius: '5px', | |||
| ...(flexGapReplacementStyle(10, false)), | |||
|
|
|||
| maxHeight: '300px', | |||
| maxHeight: maxHeightInPixel + 'px', | |||
There was a problem hiding this comment.
Editor is also using emotion-js, right? You should be able to just pass integers here. Also the borderRadius: 5 above should just work. But if the whole codebase uses + 'px' already, then it's probably not necessary to fix this for this one PR now.
There was a problem hiding this comment.
It does use Emotion, but not emotion-js specifically I think? Anyway, I like being specific about my units :D
src/main/Thumbnail.tsx
Outdated
| @@ -106,7 +107,8 @@ const Thumbnail : React.FC = () => { | |||
| discard={discardThumbnail} | |||
| /> | |||
| <div css={bottomStyle}> | |||
| <VideoPlayers refs={generateRefs} widthInPercent={100}/> | |||
| {/* use maxHeightInPixel to make video players the same size*/} | |||
| <VideoPlayers refs={generateRefs} widthInPercent={100} maxHeightInPixel={376}/> | |||
There was a problem hiding this comment.
This magic number depends on the width of the buttons. If a button is hidden (like in #1208), the width changes.
So this should likely have a different solution. Maybe both boxes just get a fixed width. Or something else.
There was a problem hiding this comment.
Welp, that teaches me to not go for the quick and dirty solution. I'll see if I can get a proper solution working, but that'll require some time.
Now actually gets the width from the video boxes. Still uses magic numbers a bit though.
|
This pull request is deployed at test.editor.opencast.org/1210/2023-11-08_08-27-25/ . |
There was a problem hiding this comment.
I am not sure this approach is the best solution. This gets the width once in the render code and is only updated on rerender. Maybe the width can change without a rerender happening? E.g. text wrapping, other CSS wrapping or screen-width based logic? I feel a fully CSS-based solution might be better, i.e. wrapping everything in a flex or grid and then let one child control the column width, the other child just filling the column.
However, it seems to work and I couldn't find any problems in practice. And I know this kind of sizing via CSS can be very tricky to get right, I struggled a lot with it and even found browser bugs. So I don't want to block merging on this theoretical concern.
|
|
||
| const { t } = useTranslation() | ||
| const theme = useTheme(); | ||
|
|
||
| // The "+40" comes from padding that is not included in the "getWidth" function | ||
| const videoWidth = generateRef ? generateRef.getWidth() + 40 : undefined |
There was a problem hiding this comment.
Would using offsetWidth instead of clientWidth remove the need to add 40 manually?
Currently in the thumbnail view, the videos are the same size as the thumbnails. However, the outlines surrounding them are different, as the outline around the thumbnails also contains buttons. This enlarges the videos to make the outlines have the same width.
Partially addresses #1207. From a usability point-of-view, I don't think making the timeline the same width as the stuff above it is currently reasonable. Since we cannot zoom into the timeline (yet™), it should be as wide as possible.