Skip to content
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

Fix: Cover Block: Unnecessary default styles for H2 heading #17815

Conversation

jorgefilipecosta
Copy link
Member

Description

Fixes: #17704

The cover block needs h2 styles to support older versions of the block, but these styles were styling current heading blocks and make the ability of themes styling the block hard.

This PR fixes this problem by changing the heading selector to rely on section.wp-block-cover-image as a parent. Versions that not had inner blocks and used an h2 heading had a section (and not a div as the current) with a class wp-block-cover-image (and not a wp-block-cover as the current). So relying on this we can apply the styles to older versions and not affect new versions.

Styles that only exist to support deprecated versions were moved and grouped together.

How has this been tested?

I used the classic editor and created a post with the following content (pasting it on Gutenberg automatically migrates to the new version of the blocks):

<!-- wp:cover-image {"url":"data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAACklEQVR4nGMAAQAABQABDQottAAAAABJRU5ErkJggg==","id":34} -->
<section class="wp-block-cover-image has-background-dim" style="background-image:url(data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAACklEQVR4nGMAAQAABQABDQottAAAAABJRU5ErkJggg==)"><h2><strong>Cover Image</strong></h2></section>
<!-- /wp:cover-image -->

<!-- wp:cover-image {"url":"data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAACklEQVR4nGMAAQAABQABDQottAAAAABJRU5ErkJggg==","id":34} -->
<div class="wp-block-cover-image has-background-dim" style="background-image:url(data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAACklEQVR4nGMAAQAABQABDQottAAAAABJRU5ErkJggg==)"><p class="wp-block-cover-image-text"><strong>Cover Block</strong></p></div>
<!-- /wp:cover-image -->

<!-- wp:cover {"url":"data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAACklEQVR4nGMAAQAABQABDQottAAAAABJRU5ErkJggg==","id":35} -->
<div class="wp-block-cover has-background-dim" style="background-image:url(data:image/png;base64,iVBORw0KGgoAAAANSUhEUgAAAAEAAAABCAYAAAAfFcSJAAAACklEQVR4nGMAAQAABQABDQottAAAAABJRU5ErkJggg==)"><p class="wp-block-cover-text"><strong>Cover Block</strong></p></div>
<!-- /wp:cover -->

I disabled classic editor, opened the post verified it was automatically migrated. Did not save the migration.
I opened the post with Gutenberg master and in this branch and verified it displays equal in both places.
I created a new cover block, I added a new heading inside the cover and verified it was not centered aligned.

@jorgefilipecosta jorgefilipecosta added [Type] Bug An existing feature does not function as intended [Block] Cover Affects the Cover Block - used to display content laid over a background image labels Oct 7, 2019
@@ -150,3 +116,42 @@
z-index: z-index(".wp-block-cover__video-background");
object-fit: cover;
}

// Styles bellow only exist to support older versions of the block.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we expand a little bit on the differences compared to the current markup and how we're using them to target only the previous versions?

Copy link
Member Author

Choose a reason for hiding this comment

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

I explained that on the description of the PR, but I agree that it should be in the comment I added it.

@m-e-h
Copy link
Member

m-e-h commented Oct 10, 2019

Is there a policy for how long we keep deprecated code? I might be wrong but if I remember correctly, h2 in the cover block was removed from the markup before the plugin was initially merged with WordPress core.

@jorgefilipecosta jorgefilipecosta force-pushed the fix/cover-block-Unnecessary-default-styles-for-H2-heading branch from 2ac86e4 to 9cc942d Compare October 10, 2019 15:33
@jorgefilipecosta
Copy link
Member Author

Is there a policy for how long we keep deprecated code?

WordPress tries to always be back-compatible so it seems core needs to always contain the styles that made a previous version of the blocks work.

I might be wrong but if I remember correctly, h2 in the cover block was removed from the markup before the plugin was initially merged with WordPress core.

I created a test site with WordPress 5.0 and verified h2 was not used in the cover. I guess in this case we can remove the h2 selectors to save some bytes. Any thoughts on this @youknowriad?

@youknowriad
Copy link
Contributor

I think we should keep them for now until we figure a better strategy. That's not the only place where we support plugin only markup. We have several blocks in that case and we even have hidden blocks that were entirely deprecated.

@jorgefilipecosta
Copy link
Member Author

Thank you for your insights @youknowriad I guess in this case the PR should be ready.

padding: $block-padding;
text-align: center;
}

h2:not(.has-text-color),
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this here?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, we need these styles because of the previous versions, they were always white. Current versions inriht the color, I move these styles to the deprecated zone.

@jorgefilipecosta jorgefilipecosta force-pushed the fix/cover-block-Unnecessary-default-styles-for-H2-heading branch from 9cc942d to a0e712a Compare October 11, 2019 11:21
@jorgefilipecosta jorgefilipecosta force-pushed the fix/cover-block-Unnecessary-default-styles-for-H2-heading branch from a0e712a to c73f4b5 Compare October 11, 2019 11:26
@jorgefilipecosta jorgefilipecosta merged commit a7cd271 into master Oct 11, 2019
@jorgefilipecosta jorgefilipecosta deleted the fix/cover-block-Unnecessary-default-styles-for-H2-heading branch October 11, 2019 11:57
@youknowriad youknowriad added this to the Gutenberg 6.7 milestone Oct 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Block] Cover Affects the Cover Block - used to display content laid over a background image [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cover Block: Unnecessary default styles for H2 heading
3 participants