Make responsive embeds a theme option #10477
Conversation
notnownikki
changed the title
|
Very nice work. I can confirm that for a theme that hasn't opted in, embeds are untouched, and that when opting in by pasting Although it would be nice if we could offer this without opt-in, I think this is the safe, and right approach. Perhaps we can remove the opt-in in a future version, and make it on by default? One can dream. And let's be sure to enable this in the Twenty themes as a "best practice", CC: @kjellr. We have 3 todo's:
Under "opt-in" features we could add: Responsive Embeds:The new editor can make sure video embeds (YouTube, Vimeo) scale responsively according to viewport and aspect ratio. This will work without issue in most themes, especially themes that don't already provide some responsive mechanims. If you'd like to offer this in your theme, you can opt into this feature by calling: add_theme_support( 'responsive-embeds' );
Question about that: is it sufficient to apply
Thanks so much for working on this Nikki! |
|
I'm not sure how to access the theme options from Gutenberg though. It's probably (hopefully) straightforward? |
|
Ah, ignore me, I haven't had coffee yet. Just need to modify the rest api to add in info about this theme option. Simple :) |
|
So I didn't go with |
|
That's wonderful. So does this also retire the body classname, or is that still necessary? Could this classname be on the block itself intead of the body tag? |
|
@jasmussen the classname is still needed to make the aspect ratio styles match, and the difference between a theme that supports and a theme that doesn't is that the one that does has the classname added to the body. That way, a theme can just ignore our aspect ratio styles if it doesn't support our method of responsiveness, or it could use them to apply its own method. Does that make sense? |
|
Yes that does make sense, thanks for the clarification. And I'm not sure the following is possible, but I'll ask regardless. Instead of adding For example right now, a random YouTube video I embed has these classes:
Could we either add an additional class to that list when the theme has opted-in, or perhaps even coopt the A theme would still be able to provide custom aspect-ratio based responsiveness based on the presence of the |
|
The problem with doing it at the block level is that we'd either need to republish every post with responsive blocks in them if the theme option changes, or parse the blocks and add the classnames based on the theme option, which is expensive to do. |
|
Given the feature has only been in one version, and that there's a fix (update post), I think that's actually fine. |
|
With this PR, we don't even need to update the posts published with the previous version. Because the stylesheet changes only to match the responsive content if the body element as the classname, then it'll just get switched off without any further changes needed. And as soon as the theme opts in, all published posts will benefit without having to republish anything. |
|
Yes I understand, and I've verified this works as well. If this is what we have to go with, that's fine. My concern is that we are stuffing additional css classes into an already loaded element. And this class would be present on every single WordPress page and post, regardless of whether it included an embed or not. It seems like it we could add this class at the block level, even if it requires updating currently published posts, this would be better for the next decade of WordPress. I just want to be sure we're looking at the long term and picking the best solution, not just the near term. Given there are three ways to fix the additional whitespace for currently published posts affected, the toggle, removing css classes, and soon just updating the post, this seems like a small price to pay for a leaner body element. |
|
I'm not sure that people who have a decade's worth of posts would agree there :) Loading each one into the editor to republish if they want to switch to a theme that supports responsive embeds? If we could only output the body classname if the current post has an embed with an aspect ratio, would that be ok? |
But how would they? Old posts aren't affected by this are they? Already now older posts would need to be converted to blocks, that seems outside the scope of this issue. |
As you said, we want to plan ahead for the next decade. If someone is using a theme that doesn't opt in for the next decade, and then wants to opt in, then they'd have that problem. |
Oh I see, right. Solid point, I'm daft. |
|
So if we only output the class on pages that have an embed, that means parsing the post to determine if the class should be there, and that's going to be a tough sell to systems people who run WordPress at scale, when we could include a few extra characters in the body element and save all those CPU cycles. |
|
Yep. Probably every page is okay. But now that I finally understood the point, I think this is open to wider feedback. |
|
I wish HTML had a better method of handling these sort of pseudo-"feature flags" but since it doesn't, a body class seems like a reasonable approach for the time being. |
There was a problem hiding this comment.
This looks good to me. I opened https://core.trac.wordpress.org/ticket/45125 to update core. The tests are failing, but it appears to be a problem upstream as well.
|
Oh boy, that |
|
@danielbachhuber I think everything has been resolved now. |
|
The docs are missing, I opened an issue: #10759 |
|
Hi, is this change going to make it into a full release? I've encountered the issue with embedded WordPress pages (such as embedding a WordPress plugin url in a page as on my page here: https://suburbia.org.uk/projects), I've had to manually disable the responsiveness on all embeds on this page to stop the gap appearing below them. But it appears this default responsive setting is incorrect at least for this type of embedded page, at least on the standard 2017 theme anyway. |
|
Yes, this will be in the 4.1 release. |
|
@notnownikki - Do you know when this enhancement will get migrated into the core (V5.0)? I will be honest...as a theme designer I am having trouble trying to keep up with the changes in two places and when to raise an issue here and when to raise an issue in Trac for the core Gutenberg. I am working with the bleeding edge core 5.0 (no Gutenberg plug-in installed) and I am finding the wp-embed-responsive class is being attached to the body for the (backend) admin edit view of a page but not the (front-end) display view of a page. Is the theme supposed to add the wp-embed-responsive class to the body in the page display view or is the core supposed to do that and its omission is just a bug? Thanks in advance. Much appreciated :-) |
|
@timhibberd I'm not sure on how things are getting merged into 5.0, to be honest. If you're seeing @pento you would know better than me how these merges are handled, would you be able to comment here please? |
|
Cheers @notnownikki - much appreciated. The Gutenberg express train is picking up steam and its leaving some of us hanging on to keep from falling off :-) Hopefully @pento can guide me w.r.t. what changes are made to the GB plug-in, what changes are made to the GB in core V5 and how they are being kept in synch. Regarding the theme opting in to wp-embed-responsive, my theme IS opting in via the So I don't know who to report this to GB plug-in or V5.0 core because I don't know where the center of truth is any more. It is possible that this feature has not yet been merged into the core V5 Beta nightly build stream. I can't tell from the Responsive Trac ticket if / when it is being merged. |
|
@timhibberd The
Notably, core development is happening on the The You'll want to check that you're running Gutenberg's latest version and the latest nightly WordPress build. If both of those are correct, it might be that your theme registration isn't working as expected. |
|
Cheers @danielbachhuber. I saw that post previously but the WordPress beta documentation says to use bleeding edge via WordPress Beta Tester plugin (which pulls the release from the trunk). So I was somewhat confused as to which instruction was accurate. So I assume the WordPress Beta Tester plugin should be left in "Point release nightlies" mode.
Also, if I pull the nightly build (wordpress-latest.zip) from the "nightly build" link on the Beta testing page (https://make.wordpress.org/core/handbook/testing/beta/ ) I get 5.1-alpha-43678 but if I do an update from the WordPress Beta Tester plugin set to "Point release nightlies" mode I get 5.0-beta1-43832. Are we leaping past 5.0 and going right to 5.1? So I'm just finding the process a bit muddled w.r.t what GB is in the core vs. what gb is in the plug-in and whether I need to pull the nightly GB plug-in release to match up with the nightly core 5.1 release and whether I still need the GB plug-in installed to test my theme on core v5.x. Thanks for clarifying that the core is adding the Much appreciated :-) |
To be honest, I'm not totally sure of the current state of the WordPress Beta Tester plugin right now. Might be worth popping into
Nope, we're shipping 5.0.
You can install the WordPress 5.0-betas to test against what will be released in 5.0. For theme development, this should be sufficient. Alternatively, if you want to test against the bleeding edge, you can install the Gutenberg nightly build (or any of the stable Gutenberg releases). |
|
Much appreciated @danielbachhuber. I got it sorted with your adept guidance. I am the custodian of the abandoned MySiteMyWay theme suite (forked as BackStop Themes in Gitlab.com). The process of modernising those themes is a gradual process. I'm Gutenising them at present. It turned out, on detailed re-examination that the original theme designers created their own html body statement in isolation and did not append the default WP body classes. Once I knew the core was definitely adding them it was simple to sort out. Thanks again for your help. Whenever I see your comments in WordPress they are always spot on. Thanks for being there when people need you...your WP knowledge & advice is invaluable :-) |
|
@timhibberd You're welcome :) |
Description
Fixes: #10109
This adds a new style,
wp-embed-responsiveto the<body>if the theme has opted in toresponsive-embeds.This allows the editor to keep the styles in the saved html and allows themes to either opt in, supply their own responsive solution, use the gutenberg provided aspect ratio styles for their own responsive solution, or to ignore them completely.
How has this been tested?
Make a new post and embed a video from youtube, e.g. https://www.youtube.com/watch?v=lXMskKTw3Bc
Check that the published post renders fine, and the body does not have the
wp-embed-responsiveclass.Opt the theme in to
responsive-embedsCheck the body has the
wp-embed-responsiveclass and the video is responsive.Types of changes
Bug fix
Checklist: