- Issue created by @tonypaulbarker
- 🇭🇺Hungary Gábor Hojtsy Hungary
Although you are linking 📌 [META] Track 15: Proposal for media management Active , I believe the description of your intended result is on 🌱 Proposal for Media Management in early versions of Drupal CMS Active , right? Do you have a checklist/summary of how much did you implement from that in this MR?
- 🇬🇧United Kingdom tonypaulbarker Leeds
@gábor-hojtsy thanks, I have added that issue to the description. I believe we have implemented everything described there once snagging is done and in addition some of the maybes and nice to haves.
This issue / MR will contain
MVP:
- Image styles and responsive image styles optimised for Olivero and plumbed in with view modes. Formats as described and using focal point for crops.
- Remote video with small, medium and large view modes
- Documents
Additionally:
- Strong media support in CKEditor including selecting view modes for size / dimensions
- SVGs with view modes for small, medium and large scaling options
- optional local audio
- optional local video
Note:
The privacy compliant embedded media described there is already included, coming from the privacy track.
- 🇺🇸United States phenaproxima Massachusetts
Reviewed this and I have two overarching concerns, one of which is commit-blocking and one of which isn't.
The blocking one is that media_base is constructed as an anti-pattern. It should not include any media types of its own; only things which may be needed by all media types. We probably will want the image media type to have its own recipe, built on top of media_base.
The non-blocking one is ZOMG I have never seen this many image styles in my entire life. Won't marketers find this overwhelming? This is an enormous amount of config, choices, and (seemingly) UI complexity to be introducing. How do we expect people to use this? I don't build sites for a living so I may very well be coming at this wrong, but this seems like it will be very intimidating for people. Should we streamline it?
- 🇬🇧United Kingdom tonypaulbarker Leeds
@phenaproxima SVG image is a separate media type because focal point can't apply any crops. We could combine it for ease of use but if you use a portrait aspect image style with a landscape aspect SVG image you will render a small image with a gap. Therefore, when an SVG image is selected in CKEditor, the editor has a choice of small, medium or large to select from, but not aspect ratios. The editor profile is not in this MR because we were waiting for https://www.drupal.org/project/drupal_cms/issues/3486284 📌 Create a new text format/editor to replace core's Basic HTML Active .
We could split up the recipe by media type. There is not much shared (only enabling responsive_image and the small, medium and large view modes so the base would be small. Shall we go ahead and do that?
Unfortunately, interacting with the image styles will not be user friendly if editors try to do that, but out of the box this will give them good options to use in their content. These are the common options that we found are presented in competitor CMS.
- 🇺🇸United States phenaproxima Massachusetts
@tonypaulbarker and I discussed this recipe on Zoom and how we think it should proceed. Here's what we landed on:
- We don't really need a media_base recipe. The only things that are shared between images and remote videos are the small, medium, and large view modes. I think it's perfectly okay for those to be duplicated if necessary.
- Let's have two recipes here: drupal_cms_image and drupal_cms_video.
- drupal_cms_images should be a maximalist recipe that contains everything needed for images to work as editors would expect them to. That means all image styles, crop types, responsive styles, view modes, etc. are contained in this recipe. It provides both the Image and SVG Image media types. Ideally those would be only one media type, but Tony explained that there are technical limitations around cropping that prevent that from working as we'd like. It's the kind of thing we could evolve in future versions of this recipe.
- drupal_cms_video will be a wrapper around core's remote_video_media_type recipe, adding the small, medium, and large view modes.
- The starter recipe will include core's audio_media_type recipe as-is, along with drupal_cms_image and drupal_cms_video.
- The starter recipe will also expose core's local_video_media_type recipe to Project Browser, for sites that want locally hosted video.
So, kicking back to "needs work" to reorganize this as we agreed. But truly this is magnificent and robust - I'm amazed by the diligence and thoughtfulness that went into this.
- 🇬🇧United Kingdom tonypaulbarker Leeds
@phenaproxima
Thanks so much for your time it was great to chat this through.
Just couple of suggested adjustments to the above - let’s call the recipe ‘drupal_cms_remote_video’ to distinguish from the other video media type that uses files
Shall we make hosted audio from core optional the same way as hosted video? Not everyone is using that either. - 🇺🇸United States phenaproxima Massachusetts
Sure, that sounds good. I'll adjust my comment.
- 🇬🇧United Kingdom tonypaulbarker Leeds
Known blockers are resolved.
We didn't get to complete this yet, because we need to work out how to show these up in CKEditor once they have been added: "The starter recipe will also expose core's local_video_media_type and audio_media_type recipes to Project Browser, for sites that want locally hosted video and audio." I will create a new issue to follow up with that.
- 🇺🇸United States phenaproxima Massachusetts
I think this looks really good overall. It's super thorough and the display options it will provide make sense and have a good variety of options for editors. I think the Image recipe will be widely adopted outside of Drupal CMS, thanks to its robustness and coherence. I feel like a thoughtful, careful approach was taken to this architecture, and it shows. Great work!
The problems, such as they are, seem to mostly be inherited from core's limitations. The coupling to Olivero is a problem, and it means that the recipe needs to outright install and depend on Olivero. Either that, or we need to move all the Olivero-specific stuff into the starter recipe, which will both reduce the usefulness of the Image recipe and blow up the size of the starter -- both bad outcomes. So for the moment, as long as we file a follow-up to fix it, I think it's okay for this recipe to land with a hard dependency on Olivero. IMHO what we need to do is make the image recipe dynamic -- either by a recipe input or a specialized config action -- so that it sets up all the responsive image styles with the proper theme and breakpoints. Chances are we can make that happen with a polyfill module (which we are likely going to need to create anyway, for unrelated reasons).
- 🇺🇸United States phenaproxima Massachusetts
This needs a follow-up to discuss how best to decouple of the responsive image styles from Olivero.
- 🇬🇧United Kingdom tonypaulbarker Leeds
@phenaproxima one of the options on this issue is to move and improve the breakpoints specifically for Drupal CMS https://www.drupal.org/project/drupal_cms/issues/3488359 ✨ Support retina devices and improve responsive image styles on mobile Needs review
- 🇺🇸United States phenaproxima Massachusetts
Adding that as a related issue; should we maybe rescope it to be about decoupling the responsive image styles from Olivero altogether? That way we don't need a separate follow-up issue to commit this.
- 🇺🇸United States phenaproxima Massachusetts
Looks like there are some outstanding merge conflicts...?
- 🇬🇧United Kingdom tonypaulbarker Leeds
@phenaproxima current conflicts are resolved. Re-tested some things and all looks well.
- 🇺🇸United States phenaproxima Massachusetts
Only three small points, two of which are probably just an oversight. Then this is RTBC.
- 🇺🇸United States phenaproxima Massachusetts
Follow-ups are opened and this is RTBC to me from a code perspective. Assigning to @pameeela for final validation.
- 🇺🇸United States phenaproxima Massachusetts
I spoke to Pam over Zoom and she authorized me to merge this as-is. She will manually test it once it's in and we can make tweaks as we need to.
I'm okay with that, so this is a-goin' in.
-
phenaproxima →
committed 51076904 on 0.x authored by
tonypaulbarker →
Issue #3487391 by tonypaulbarker, phenaproxima, gábor hojtsy: Add SVG...
-
phenaproxima →
committed 51076904 on 0.x authored by
tonypaulbarker →
- 🇺🇸United States phenaproxima Massachusetts
It's been a long road, getting from there to here, but we've finally taken this first, monumental step. Merged into 0.x!
Automatically closed - issue fixed for 2 weeks with no activity.