Fix video fields inside field collections - #4064
Conversation
|
All contributors have signed the CLA ✍️ ✅ |
|
There was a problem hiding this comment.
🟡 Changes recommended
The field-collection regression is only manually verified and has no automated test.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Verdict: Needs changes. The change addresses the root cause by moving the video modal outside the field-collection form context, but lacks automated regression coverage.
Changes:
- Replaces locally rendered
VideoModalstate withuseVideoModal. - Preserves video settings, disabled state, and save behavior.
- Removes the nested modal from
VideoFooter.
The boundary and existing caller are correctly handled, with no public API break identified. Documentation is unnecessary for this internal bug fix; the remaining risk is regression without a targeted component test.
File summaries
| File | Description |
|---|---|
assets/js/src/core/modules/element/dynamic-types/definitions/objects/data-related/components/video/footer.tsx |
Opens video settings through the global modal provider. |
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 0
- Review effort level: Balanced
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
I have read the CLA Document and I hereby sign the CLA |
There was a problem hiding this comment.
🟡 Changes recommended
The behavioral fix lacks a regression test covering video fields inside field collections.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 1/1 changed files
- Comments generated: 1
- Review effort level: Balanced
| export const VideoFooter = (props: VideoFooterProps): React.JSX.Element => { | ||
| const { t } = useTranslation() | ||
| const [isModalVisible, setIsModalVisible] = useState(false) | ||
| const { openModal } = useVideoModal({ |
There was a problem hiding this comment.
@pverdu-emico
Hi! Thanks a lot for bringing us this fix. Everything looks good already! There are just two small things I'd like to ask you to take care of:
- Could you please rebase the changes onto the 2026.2 branch?
- Could you also take a look at this comment?
Thanks in advance! :)
Changes in this pull request
Fixes pimcore/platform-version#443
Fixes video fields that are used inside field collections.
The video settings modal was rendered inside the field collection's numbered form context. This caused the modal fields to be handled as field collection fields, resulting in an empty video ID and an
about:blankpreview.The video field now opens the modal through the existing
VideoModalProvider. This renders the modal outside the field collection while preserving the existing behaviour for regular video fields.Additional info
Verified in a local Pimcore setup with a YouTube video inside a field collection. The preview now loads the expected
youtube-nocookie.comembed URL.npm run check-types