Embed media in rich text fields using Draftail #90
Conversation
glitchwolfe
force-pushed
the
master
branch
5 times, most recently
from
a57d9a6
to
5f16dc4
Compare
- Update tests for rich text chooser - Address inconsistent tab/spacing issues - Address more CI issues - Properly sort imports - For wagtail versions <2.5, use MediaEmbedHandler instead of EmbedHandler - In Wagtail <2.5, specify embed type and handler explicitly for register_embed_type
There was a problem hiding this comment.
Love it! It’s quite a lot of work, and as you said there are a few caveats / rough edges, but getting this up and running locally it works as expected.
Signed URLs feels like the most problematic of all issues you’ve faced – but to be honest I’m not sure I understand why that’s not a problem regardless of your PR? Aren’t the URLs stored at the time of saving the media regardless?
- For styles, I think further work is needed, but you’re right in wanting to keep this to a minimum. I think we could reuse Wagtail’s EmbedBlock for the rich text editor, and that might solve a few problems for us.
- For icons, we should use SVGs (inline might be the best for the current purpose). That’s what Wagtail is going to switch to, which should make wagtailfontawesome redundant.
Finally, one important question – currently you have this as a single "Media" feature. Why not two separate "Audio" and "Video" features? I can see how it would be convenient to be able to say that a field should contain audio, or video, or both. It also seems like a good idea to keep it together for consistency with the rest of wagtailmedia, so not too sure what to think of this.
And finally – you’ve made those things block-level entities in Draftail. Would there ever be a case where it would be valuable to have these as inline entities (that can be inserted inside text)?
|
|
||
| 'autoplay': props.get('autoplay'), | ||
| 'mute': props.get('mute'), | ||
| 'loop': props.get('loop'), |
There was a problem hiding this comment.
I wonder if it might be better to just serialize all the props as a single JSON-encoded attribute, rather than have to store them as separate HTML attributes like this. Then you wouldn’t have to do the annoying True if attrs.get('autoplay') == 'true' else False conversion back the other way around.
| Form for customizing media player behavior (e.g. autoplay by default) | ||
| prior to insertion into a rich text area | ||
| """ | ||
| autoplay = forms.BooleanField(required=False) |
There was a problem hiding this comment.
I wouldn’t ever recommend anyone make anything autoplay, although I know some people might. How can we make the presence of this field configurable?
| if(data.autoplay) options.push("Autoplay"); | ||
| if(data.mute) options.push("Mute"); | ||
| if(data.loop) options.push("Loop"); | ||
| const options_str = options.length ? options.join(", ") : ""; |
There was a problem hiding this comment.
Not to worry about right now but these strings should be translate-able. We can implement this similarly to Wagtail.
| } | ||
| else if(data.type == 'audio'){ | ||
| icon = React.createElement('span', { class:"icon icon-fa-music", 'aria-hidden':"true" }); | ||
| } |
There was a problem hiding this comment.
Draftail supports providing icons as SVG so we don’t need font awesome for these at least.
|
|
||
| return React.createElement('button', | ||
| { | ||
| class: 'MediaBlock WagtailMediaBlock '+data.type, |
There was a problem hiding this comment.
Yep, it looks like we should just reuse MediaBlock.
| { class:"MediaBlock__icon-wrapper", 'aria-hidden': "true" }, | ||
| React.createElement('span', {}, icon) | ||
| ), | ||
| React.createElement('img', { src: data.thumbnail }), |
There was a problem hiding this comment.
Needs an alt="" attribute to be valid HTML
| [ | ||
| React.createElement('span', | ||
| { class:"MediaBlock__icon-wrapper", 'aria-hidden': "true" }, | ||
| React.createElement('span', {}, icon) |
There was a problem hiding this comment.
This doesn’t need aria-hidden if it’s already set on the icon.
| {% if loop %}loop{% endif %} | ||
| controls> | ||
| </audio> | ||
| {% endblock %} |
There was a problem hiding this comment.
This might be a stupid question but this project didn’t have anything like this to define the medias’ output before?
| {% if autoplay %}autoplay{% endif %} | ||
| {% if loop %}loop{% endif %} | ||
| {% if thumbnail %}poster="{{thumbnail}}"{% endif %}> | ||
| <source src="{{file}}" data-title={{title}}> |
| ContentstateMediaConversionRule | ||
| ) | ||
|
|
||
| features.default_features.append(feature_name) |
There was a problem hiding this comment.
I don’t think it should be enabled by default for all fields.
|
Is this still in progress or it's already usable? |
|
@ordigital this most definitely needs improvements. and bringing the code up-to-date with package and Wagtail changes |
But are there any working solutions for inserting wagtailmedia files to richtext editor? |
No. Most people use this in StreamField with a rich text block followed by a media block, followed by any other block, be it rich text or custom. |
Thanks. So I guess Draftail integration feature has been completely dropped? |
This was mainly reverse-engineered from Wagtail's existing Image chooser. I still think it needs some work before putting it in the hands of all users, but it is functional.
Some notes:
Signed URLs
/custommedia/thumbnails/<MEDIA_ID>and/custommedia/files/<MEDIA_ID>. These views redirect to the correct URL, and the alias can be relied upon to always be the same.wagtailmediaonly contains aadmin_urlhook into Wagtail (and not regular URLs), I am not certain of the best way to approach this without requiring all users to add these URLs to their project manually.Styles
This code won't look the same as it did in my preview screenshots from my feature request.
I noticed this project doesn't have any CSS, and I didn't want to be the first to include it. I did this for the sake of keeping this PR simple - if there are existing Wagtail components we can use to improve things without custom HTML/CSS, I'm all for it (if there's a desire for anything specific from those screenshots, I'll happily go back in and port that code over as well).
The only styles I've added are an inline min height/width of
100pxfor the media file preview in the text editor. Everything else uses existing class names within Wagtail. Well, except for...Icons
Wagtailfontawesomefor the video/audio icons. I'm not sure what the best replacement would be for these, since wagtail doesn't provide any icons that would work well by default.