Skip to content

Embed block: Don't rely on a global wp-embed-responsive class #33516

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.

Sign up for GitHub

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

Closed
walbo wants to merge 2 commits into from
Closed

Embed block: Don't rely on a global wp-embed-responsive class #33516

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3c43883
Embed block: Don't reply on a global `wp-embed-responsive` class
Jul 16, 2021
10c423b
Add inline comment
Jul 16, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 2 additions & 3 deletions lib/widgets-page.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -139,14 +139,13 @@ function gutenberg_widgets_editor_load_block_editor_scripts_and_styles( $is_bloc
* Adds admin classes necessary for the block-based widgets screen.
*
* - Adds `block-editor-page` editor body class to allow directly styling the admin pages that are based on the block editor.
* - Shows responsive embeds correctly on the widgets screen by adding the `wp-embed-responsive` class.
*
* @param string $classes existing admin body classes.
*
* @return string admin body classes including the `block-editor-page` and `wp-embed-responsive` classes.
* @return string admin body classes including the `block-editor-page` class.
*/
function gutenberg_widgets_editor_add_admin_body_classes( $classes ) {
return "$classes block-editor-page wp-embed-responsive";
return "$classes block-editor-page";
}

/**
Expand Down
5 changes: 0 additions & 5 deletions packages/block-editor/src/components/iframe/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -154,11 +154,6 @@ function setBodyClassName( doc ) {
for ( const name of document.body.classList ) {
if ( name.startsWith( 'admin-color-' ) ) {
doc.body.classList.add( name );
} else if ( name === 'wp-embed-responsive' ) {
// Ideally ALL classes that are added through get_body_class should
// be added in the editor too, which we'll somehow have to get from
// the server in the future (which will run the PHP filters).
doc.body.classList.add( 'wp-embed-responsive' );
}
}
}
Expand Down
3 changes: 2 additions & 1 deletion packages/block-library/src/embed/edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,8 @@ const EmbedEdit = ( props ) => {
}
}, [ preview, isEditingURL ] );

const blockProps = useBlockProps();
// Shows responsive embeds correctly by adding the `wp-embed-responsive` class.
const blockProps = useBlockProps( { className: 'wp-embed-responsive' } );
Copy link
Member

@kevin940726 kevin940726 Aug 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix makes sense to me, but if we always want it to be responsive, then what's the point of this class? Could we remove it entirely? @ellatrix WDYT?

Copy link
Member

@ellatrix ellatrix Aug 3, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me. I don't know what this class is for, I don't know much about the embed block.

Copy link
Member Author

@walbo walbo Aug 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The aspect ratio css needs the class in:

gutenberg/packages/block-library/src/embed/style.scss

Lines 42 to 89 in b72adf8

// Add responsiveness to embeds with aspect ratios.
.wp-embed-responsive .wp-has-aspect-ratio {
.wp-block-embed__wrapper::before {
content: "";
display: block;
padding-top: 50%; // Default to 2:1 aspect ratio.
}
iframe {
position: absolute;
top: 0;
right: 0;
bottom: 0;
left: 0;
height: 100%;
width: 100%;
}
}
.wp-embed-responsive {
.wp-embed-aspect-21-9 .wp-block-embed__wrapper::before {
padding-top: 42.85%; // 9 / 21 * 100
}
.wp-embed-aspect-18-9 .wp-block-embed__wrapper::before {
padding-top: 50%; // 9 / 18 * 100
}
.wp-embed-aspect-16-9 .wp-block-embed__wrapper::before {
padding-top: 56.25%; // 9 / 16 * 100
}
.wp-embed-aspect-4-3 .wp-block-embed__wrapper::before {
padding-top: 75%; // 3 / 4 * 100
}
.wp-embed-aspect-1-1 .wp-block-embed__wrapper::before {
padding-top: 100%; // 1 / 1 * 100
}
.wp-embed-aspect-9-16 .wp-block-embed__wrapper::before {
padding-top: 177.77%; // 16 / 9 * 100
}
.wp-embed-aspect-1-2 .wp-block-embed__wrapper::before {
padding-top: 200%; // 2 / 1 * 100
}
}

Removing the classname from backend would require to duplicate the aspect ratio css to editor.scss and remove the required wp-embed-responsive. Don't think we can change style.scss because responsive embeds are opt-in for themes.

Copy link
Member

@ellatrix ellatrix Sep 1, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But we are hardcoding the class here? How do we make it opt-in for themes in the editor?


if ( fetching ) {
return (
Expand Down