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.

Sign up for GitHub

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

Updated video dimensions to fit view horizontal/vertical #10101

Merged
merged 8 commits into from
Dec 8, 2023
Merged

Updated video dimensions to fit view horizontal/vertical #10101

Changes from 2 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
d35a927
update video dimensions to fit view horizontal/vertical
jacob-nv Nov 30, 2023
9423ab9
Merge branch 'master' into vertical-video-fix
jacob-nv Nov 30, 2023
7c5c9a5
updating mediavideo to composition api
jacob-nv Dec 1, 2023
f3ca42d
Merge branch 'master' into vertical-video-fix
jacob-nv Dec 1, 2023
5d7dc55
updating video ref types, video dimensions changelog
jacob-nv Dec 5, 2023
ddec04a
type & lint fix MediaVideo
jacob-nv Dec 6, 2023
4af20d1
type fix MediaVideo
jacob-nv Dec 6, 2023
5f4ef3a
Apply suggestions from code review
kulmann Dec 8, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 25 additions & 1 deletion packages/web-app-preview/src/components/Sources/MediaVideo.vue
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,12 @@
<template>
<video :key="`media-video-${file.id}`" controls preload="preload" :autoplay="isAutoPlayEnabled">
<video
ref="video"
:key="`media-video-${file.id}`"
controls
preload="preload"
:autoplay="isAutoPlayEnabled"
@loadedmetadata="updateDimensions"
>
<source :src="file.url" :type="file.mimeType" />
</video>
</template>
Expand All @@ -18,6 +25,23 @@ export default defineComponent({
type: Boolean,
default: true
}
},
beforeUnmount() {
window.removeEventListener('resize', this.resizeVideoDimensions)
},
methods: {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use the composition API by adding the methods and onBeforeUnmount in setup.

updateDimensions() {
this.resizeVideoDimensions()
window.addEventListener('resize', this.resizeVideoDimensions)
Copy link
Contributor

Choose a reason for hiding this comment

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

Each time this method gets called you register an event listener, is this done intentionally? Wouldn't it make more sense to register the event listener once on mount?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh.. my bad, you are right. At first I was working with the videoWidth and videoHeight, then change it to setting max dimensions and didn't update this. Yeah, thats completely reasonable to set in onMounted

},
resizeVideoDimensions() {
const maxHeight = document.querySelector('.stage_media')?.offsetHeight - 10 ?? null
const maxWidth = document.querySelector('.stage_media')?.offsetWidth - 10 ?? null
if (maxHeight && maxWidth && this.$refs.video) {
this.$refs.video.style.maxHeight = `${maxHeight}px`
this.$refs.video.style.maxWidth = `${maxWidth}px`
}
}
}
})
</script>