Skip to content

Conversation

@walbo
Copy link
Member

@walbo walbo commented Jul 16, 2021

Description

Move the wp-embed-responsive class to embed block. This makes sure the embed block isn't dependent on a global classname. The current implementation requires the classname to be added on every screen the editor is used.

Related: #32057 (comment)

Fixes: https://core.trac.wordpress.org/ticket/53609

How has this been tested?

Tested embeds in:

  • Post editor
  • Template editor
  • Widget
  • Customizer widgets

Screenshots

Types of changes

Bug fix

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • I've tested my changes with keyboard and screen readers.
  • My code has proper inline documentation.
  • I've included developer documentation if appropriate.
  • I've updated all React Native files affected by any refactorings/renamings in this PR (please manually search all *.native.js files for terms that need renaming or removal).

Petter Walbø Johnsgård added 2 commits July 16, 2021 18:16
Move the `wp-embed-responsive` class to embed block. This makes sure the embed block isn't dependent on a global classname. The current implementation requires the classname to be added on every screen the editor is used.
@walbo walbo changed the title Embed block: Don't reply on a global wp-embed-responsive class Embed block: Don't rely on a global wp-embed-responsive class Jul 16, 2021
@annezazu annezazu added [Block] Embed Affects the Embed Block [Type] Bug An existing feature does not function as intended labels Jul 28, 2021

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

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

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

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:

// 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

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?

@skorasaurus
Copy link
Member

Hi, this has been implemented in Core so the PR, so is this PR still relevant? Please reopen and modify it it is.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

[Block] Embed Affects the Embed Block [Type] Bug An existing feature does not function as intended

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants