Add option to make embeds responsive

For folks using the patch from ✨ CKEditor 5 Support Fixed , here is a patch that can be applied alongside that one. This will not apply directly to the 2.x branch on its own.

I tested #46 with the CkEditor 5 patch and it works well, thank you.

segovia94

@2dareis2do The inline padding is what allows the iframe to be responsive by setting it to a ratio of the width. This keeps the aspect ratio of the embed consistent no matter the screen size. See https://blog.theodo.com/2018/01/responsive-iframes-css-trick/ for the css trick used to accomplish this.

Totally agree with the technique. If i remember correctly there are similar alternative techniques for getting the aspect ratio correct. My point is that css should be in the css, not inline, in order to be responsive.

I adjusted @segovia94's patch to work on version 2.0@alpha

Last diff didn't include all files. Attached another attempt.

Here's a patch updated for 1.x, I've replaced deprecated function drupal_get_path

Last diff didn't include the libraries yml file...

Sorry for the many retries. I haven't done this in a while. This patch should work against the 2.x-dev version

Sorry for the many retries. I haven't done this in a while. This patch should work against the 2.x-dev version

This needs to be updated for the committed ckeditor5 integration I guess.

FWIW, AFAIK modern css allows for a different and better way to define that.

How does core handle that? do we really need to ship the CSS for that ourself or is there something from core that we can use?

How does core handle that? do we really need to ship the CSS for that ourself or is there something from core that we can use?

As far as I'm aware, this is still in-progress in core. The relevant issue is 🐛 Some oEmbed videos do not maintain aspect ratio Needs work , where the CSS approach proposed for url_embed matches that proposed 3 years ago ( https://www.drupal.org/project/drupal/issues/3060968#comment-13959443 🐛 Some oEmbed videos do not maintain aspect ratio Needs work ), and that appeared to be the direction core was going to go, per https://www.drupal.org/project/drupal/issues/3060968#comment-14588388 🐛 Some oEmbed videos do not maintain aspect ratio Needs work .

However, more recently, there is a proposal in https://www.drupal.org/project/drupal/issues/3060968#comment-14968763 🐛 Some oEmbed videos do not maintain aspect ratio Needs work to make use of the aspect-ratio property, which as this point is well-supported by browsers: https://caniuse.com/mdn-css_properties_aspect-ratio

So we should probably explore that option before committing to the previous approach.

This patch should work against the 2.x-dev version. I've tested with url_embed 2.0.0-alpha2 release.

Sorry, that last patch won't do anything for making this module work responsively. But this patch should. I've tested with url_embed 2.0.0-alpha2 release.

This patch contains the new files. I've tested with url_embed 2.0.0-alpha2 release.

#59 does not apply over 2.0.0-alpha2 release.
#58 applies, but it crashes because file /templates/responsive-embed.html.twig from #59 is not included in the patch #58.

Agreed that there are problems with both patches in #58 and #59. The attached patch will cleanly apply to 2.0.0-alpha2. Since there are subsequent changes in the 2.x branch that require adjustment, I'll update the merge request to be up-to-date with 2.x, as well.

Note that this patch does not provide a newer methodology as proposed in https://www.drupal.org/project/url_embed/issues/2825602#comment-15383198 ✨ Add option to make embeds responsive Needs work . This patch is intended to provide a solution for people trying to update to 2.0.0-alpha2 immediately.

Setting to "Needs work," the goal being to update the existing (working) merge request to use a more modern CSS technique, as suggested in Comment #55 and elaborated on in Comment #56.

As I think about this further, I think this responsive embed option should be enabled by default for new integrations.

This is the exception to the rule for new configuration options, of course, but in this case it makes sense: I believe that responsive display is going to be desired in the majority of cases, and opting out of that behavior the minority (as in the case where a site has customized the display further).

Another reason that I think it should be enabled by default is that the configuration option will be in one of those places in the Drupal interface that no one thinks to look: in a vertical tab as a setting under enabled text format filters. When working with my colleague recently on this, he "couldn't get responsive embeds" to work multiple times because he didn't notice the setting. We can and should and will document this, but we all know how many people read the manual...

I'll update the MR along these lines, but open to contrary opinions!

Testing the change in the merge request, I find that I can't see an embedded Soundcloud file, either in the CKEditor window or on the page. The wrapping element around the <iframe> needs to have some value for "padding-bottom," but instead has a NULL value, so the display appears invisible.

I find that I can't see an embedded Soundcloud file, either in the CKEditor window or on the page

Thanks for reporting this! Commit https://git.drupalcode.org/project/url_embed/-/merge_requests/9/diffs?co... handles this scenario.

I would recommend moving the responsive css away from the previous "padding" hack to the widely supported aspect-ratio property. I'm happy to update the code to use it unless there is some legacy reason not to. The previous padding hack can create problems due to its use of absolute positioning.

I would recommend moving the responsive css away from the previous "padding" hack to the widely supported aspect-ratio property. I'm happy to update the code to use it unless there is some legacy reason not to.

I think that comports with the feeling in the comment thread above, e.g., https://www.drupal.org/project/url_embed/issues/2825602#comment-15385474 ✨ Add option to make embeds responsive Needs work

Yes, please feel free to push that code change to the merge request! There is no legacy reason to preserve the padding hack, in my opinion, since this has not been merged into the module yet.

Thanks!

Thanks, @segovia, for the more modern approach to the responsive embed in https://git.drupalcode.org/project/url_embed/-/merge_requests/9/diffs?co... !

With a few adjustments, tests pass. This is good to add to the 3.x branch, with the understanding that it is designed as an opt-in feature, so it will not negatively affect sites already using this module.

Automatically closed - issue fixed for 2 weeks with no activity.