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.

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

Remove block editor responsive embed styling #1683

Merged
merged 7 commits into from Dec 1, 2018

Conversation

3 participants
@kienstra
Collaborator

kienstra commented Nov 30, 2018

  • There's a wrapper for some embed-type blocks, like YouTube, Vimeo, and SoundCloud.
  • It wraps the block in a div.wp-block-embed__wrapper, and there's a style rule that gives this a padding-top.
  • In AMP, this adds a large gap at the top of the block.
Override respsonsive embed styling in Gutenberg
There's a wrapper for some embed-type blocks,
like YouTube, Vimeo, and SoundCloud.
It wraps the block in a div.wp-block-embed__wrapper
And there's a style rule that gives this a padding-top.
In AMP, this adds a large gap at the top of the block.
So add a style rule that overrides this.
@kienstra

This comment has been minimized.

Collaborator

kienstra commented Nov 30, 2018

Steps To Reproduce

Thanks to @amedina for pointing this out.

  1. Create a post using the block editor
  2. Add a YouTube block with https://www.youtube.com/watch?v=XOY3ZUO6P0k
  3. Visit the front-end in Paired or Native mode
  4. Expected: there's not a gap above the content
  5. Actual: there's a large gap above the content

gap-above-content

https://native-ampconfdemo.pantheonsite.io/2018/11/30/embed-test/

Override another style rule that affects embed-type blocks that rende…
…r as an <amp-frame>

For example, Spotify and WordPress TV.
The <amp-iframe> usually has a layout of relative.
@kienstra

This comment has been minimized.

Collaborator

kienstra commented Dec 1, 2018

Before

before-branch

After

with-branch

Correct comment describing styling
It should have been 'position of relative',
not 'layout of relative'
* Override responsive embed styling from the block editor.
* @link https://github.com/WordPress/gutenberg/blob/9a16ac09ddff4b0bf12430d1426c4cfefa75b56d/packages/block-library/src/embed/style.scss#L52
*/
body.wp-embed-responsive .wp-block-embed[class*="wp-embed-aspect-"] .wp-block-embed__wrapper::before {

This comment has been minimized.

@kienstra

kienstra Dec 1, 2018

Collaborator

The [class*="wp-embed-aspect-"] definitely isn't ideal. But it looks like it's needed to be more specific than the style rule it overrides, like:

.wp-embed-responsive .wp-block-embed.wp-embed-aspect-16-9 .wp-block-embed__wrapper:before {
    padding-top: 56.25%;
}
@kienstra

This comment has been minimized.

Collaborator

kienstra commented Dec 1, 2018

Request For Review

Hi @westonruter and @amedina,
Could you please review this? Sorry for late Friday code reviews 😄

@kienstra kienstra changed the title from [WIP] Override respsonsive embed styling in Gutenberg to Override respsonsive embed styling in Gutenberg Dec 1, 2018

kienstra added some commits Dec 1, 2018

Revert additions to amp-default.css
As Weston mentioned, we can simply strip the
wp-embed-aspect- class.
Remove any style rule with a class beginning with 'wp-embed-aspect-'
Like wp-embed-aspect-21-9
As Weston mentioned,
this approach will be better than overriding styling from this.
Improve comment, stating that it removes any rule with the class
It doesn't simply remove the class from a rule,
it removes the entire rule that has the class.
@kienstra

This comment has been minimized.

Collaborator

kienstra commented Dec 1, 2018

Location Of Change

Hi @westonruter,
The stripping of the rule with a wp-embed-aspect-* class in 4b6341b might not be in the right place.

Another benefit of your idea is it'll remove some unnecessary styling, freeing a little more space in <style amp-custom>:

css-rules

@kienstra kienstra changed the title from Override respsonsive embed styling in Gutenberg to Remove block editor responsive embed styling Dec 1, 2018

@kienstra kienstra added this to Ready for review in v1.0 Dec 1, 2018

@kienstra kienstra requested a review from westonruter Dec 1, 2018

* Remove any rule with a responsive styling class for blocks, which isn't needed in AMP.
* @link https://github.com/WordPress/gutenberg/blob/9a16ac09ddff4b0bf12430d1426c4cfefa75b56d/packages/block-library/src/embed/style.scss#L26
*/
if ( preg_match( '/wp-embed-aspect-\S/', $selector ) ) {

This comment has been minimized.

@westonruter

westonruter Dec 1, 2018

Member

This works, but I was thinking that the underlying class name could be removed from the block HTML element itself. The CSS would then be stripped as a matter of course by the tree shaker.

@kienstra

This comment has been minimized.

Collaborator

kienstra commented Dec 1, 2018

Here's how embed-type blocks look with this PR applied:

removed-style-rule

@kienstra

This comment has been minimized.

Collaborator

kienstra commented Dec 1, 2018

Approved

Hi @westonruter,
Thanks a lot for fixing this.

It looks like I can't approve this because I opened the PR.

With this PR, the wp-embed-aspect-* class doesn't appear in elements or style rules:
amp-embed-url

'<figure class="wp-block-embed"><amp-facebook></amp-facebook><amp-facebook></amp-facebook></figure>',
'<figure class="wp-block-embed"><amp-facebook></amp-facebook><amp-facebook></amp-facebook></figure>',
'<figure class="wp-block-embed wp-embed-aspect-16-9 wp-has-aspect-ratio"><amp-facebook></amp-facebook><amp-facebook></amp-facebook></figure>',
'<figure class="wp-block-embed wp-has-aspect-ratio"><amp-facebook></amp-facebook><amp-facebook></amp-facebook></figure>',

This comment has been minimized.

@kienstra

kienstra Dec 1, 2018

Collaborator

Nice, it's good to have a test for this.

@westonruter westonruter merged commit 5aa2f86 into 1.0 Dec 1, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@westonruter westonruter deleted the fix/embed-wrapper branch Dec 1, 2018

@kienstra kienstra moved this from Ready for review to Ready for Merging in v1.0 Dec 5, 2018

@miina miina moved this from Ready for Merging to In Production in v1.0 Dec 11, 2018

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