feat(#607, #608): add support for labels with audio and video#611
feat(#607, #608): add support for labels with audio and video#611latin-panda merged 19 commits intomainfrom
Conversation
🦋 Changeset detectedLatest commit: 2df4e41 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
| const parentPath = localPath.replace(/\/[^/]+$/, ''); | ||
|
|
||
| service.activateFixtures(parentPath, ['file', 'file-csv', 'images']); | ||
| service.activateFixtures(parentPath, ['file', 'file-csv', 'images', 'audio', 'video']); |
There was a problem hiding this comment.
The changes in this file are to support audio and video in test forms (local dev and demo page on the website)
| this.mimeType = 'image/svg+xml'; | ||
| break; | ||
|
|
||
| case '.mp4': |
There was a problem hiding this comment.
The changes in this file are to support mp3 and mp4 in test forms (local dev and demo page on the website)
| align-content: flex-start; | ||
| flex-wrap: wrap; | ||
|
|
||
| :deep(.media-content) { |
There was a problem hiding this comment.
align video and audio media
| flex-direction: column; | ||
| align-items: center; | ||
| justify-content: flex-start; | ||
| gap: 10px; |
There was a problem hiding this comment.
Adding space in case of more than one media (edge case) in the same option
| import { type ObjectURL } from '@getodk/common/lib/web-compat/url.ts'; | ||
| import { computed, ref, triggerRef } from 'vue'; | ||
|
|
||
| defineProps<{ |
There was a problem hiding this comment.
Mostly preexisting code that was split between ImageBlock and MediaBlockBase
| 'broken-file': errorMessage?.length, | ||
| }" | ||
| > | ||
| <slot |
There was a problem hiding this comment.
Mostly preexisting code from the original ImageBlock component, with a slot element added to pass the file and render it.
| @change="$emit('change')" | ||
| /> | ||
| <TextMedia :label="option.label" /> | ||
| <TextMedia :label="option.label" :audio-icons-only="question.currentState.isSelectWithImages" /> |
There was a problem hiding this comment.
The engine detects if a select contains at least one image, and it displays differently from regular selects (each option is a card). Selects with images only show the play and stop buttons for audio, positioned next to the label, while videos are displayed in the default browser player.
|
@sadiqkhoja This is ready for review. Let me know any questions. Thanks for the help! |
sadiqkhoja
left a comment
sadiqkhoja
left a comment
There was a problem hiding this comment.
looks good. I have added couple of questions for clarification, none of them should block this PR
| import { ref } from 'vue'; | ||
|
|
||
| defineProps<{ | ||
| readonly resourceUrl?: JRResourceURL; |
There was a problem hiding this comment.
can resourceUrl ever be null? Looking at its usage in TextMedia and ControlLabel, I am not sure if typescript is smart enough to know that if resourceUrl is null then component is not rendered at all.
same comment applies to ImageBlock and VideoBlock
There was a problem hiding this comment.
Good question! This line was incorrectly copied from the image component. The audio and video should have a defined resourceURL (I've removed the ? operator). If it is undefined or null, the component won't display. It's considered that no media were defined.
The image component renders from a resource URL or a blob.
…-label # Conflicts: # README.md
2 participants
Closes #607 #608
I have verified this PR works in these browsers (latest versions):
What else has been done to verify that this works as intended?
Test mobile: Label with media
Test desktop: Label with media
Regression test mobile: image upload
Regression test desktop: image upload
Regression test mobile: select with image
Regression test desktop: select with image
Why is this the best possible solution? Were any other approaches considered?
The engine already supported video and audio within iText elements used for labels and
<select>options. This PR focuses on building support in the client side (Vue app)It reuses the existing logic for images and introduces a new base component called
MediaBlockBase, which is responsible for:We now have three specialized components, one for each media type, that implement
MediaBlockBase, receive the file, and handle rendering:ImageBlockReuses the pre-existing logic that determines whether an image is "small" and adjusts its display accordingly
VideoBlockRenders the browser's native
<video>playerAudioBlockRenders the browser's native
<audio>playerAdditionally provides play/stop controls when only the icon is displayed (without the full player UI)
The native browser players are used with their default styling (which varies between browsers).
This is intentional for the current implementation as we aimed for the leanest possible solution.
A custom player may be integrated in the future.
The
autoplaywill be implemented as part of another ticket once the pagination has been implemented.How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?
None. The images should continue working the same way. A regression test has been done in: labels, select options, and the upload question type.
Do we need any specific form for testing your changes? If so, please attach one.
The PR includes a test form.
What's changed
.mp4and.mp3attachments. This is only for allowing test forms to find the attachment and render it (for local development and demo site on the website)