-
Notifications
You must be signed in to change notification settings - Fork 3
feat: Add Custom Media Embed Plugin for Tiptap Editor #2711
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.
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
Conversation
frontends/ol-components/src/components/TiptapEditor/components/tiptap-ui/media-embed/lib.ts
Fixed
Show fixed
Hide fixed
OpenAPI ChangesShow/hide 7 changes: 7 error, 0 warning, 0 infoUnexpected changes? Ensure your branch is up-to-date with |
2ae4499 to
e2e72fd
Compare
jonkafton
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The non-library code change looks clean and it's good to see that we can add plugin functionality with minimal code change.
Hit the following url '/articles/new' rest flow you can easily follow
This wasn't entirely true, for me at least. Could you detail the steps to test so it's clear what functionality to expect.
Some issues:
-
From https://github.com/mitodl/hq/issues/9276:
Add input UI (modal, slash command, toolbar button, etc.) for users to enter/paste video URLs.
We have the new "Embed" button in the toolbar, but no slash command - not sure if that's a hard requirement for this PR but we have some demo code (link) if we do want to include.
Use oEmbed or provider-specific logic to render the player.
Is any renderer included? We are looking into using Video.js to render the video shorts section. This would be useful here for a consistent player to smooth differences between provider sources.
-
We ideally don't want to use the browser window.prompt() interface to capture user input. Check e.g. the UpsertUserListDialog for input modal ol-components use and style.
-
YouTube Shorts share links (
https://www.youtube.com/shorts/VIDEO_ID) cannot be embedded. I get error:"Refused to display 'https://consent.youtube.com/' in a frame". This could be a region/GDPR constraint. The embed format does work - we should rewrite tohttps://www.youtube.com/embed/VIDEO_ID. -
The
.media-containerclass sets a height of 100vh (full window height). They are therefore huge on the page and not contained between the header and footer. This is true for all layout variants. 16:9 wide videos are scaled down to the container width, but leaving a lot of vertical space.
|
4e1acd3 to
c95d7bf
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good 👍
We were hoping to avoid changes to vendor code to ease upgrades, though I'm not sure how practical that is given TipTap's workflow that adds base code to the project. We should extend where possible or consider refactoring extension code into the project. I do want to better delineate the vendor code by putting it in its own folder.
A merge from main should fix the failing OpenAPI diff job.

What are the relevant tickets?
In reference to https://github.com/mitodl/hq/issues/9276
Description (What does it do?)
We want to provide content creators with the ability to insert and customize embedded media inside the Tiptap editor.
The custom plugin should support adding video URLs, resizing the embedded player for different screens like medium.com has for media.
Screenshots (if appropriate):
How can this be tested?