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

Cover block: fix duotone when background is set to fixed #40554

Merged
merged 11 commits into from
May 24, 2022

Conversation

matiasbenedetto
Copy link
Contributor

@matiasbenedetto matiasbenedetto commented Apr 22, 2022

What?

Fixes: #31662

Cover block: fix duotone when the background is set to fixed

Thanks, @ajlende for your guidance here!

Why?

Currently, the cover duotone breaks if the content is set to fixed

How?

I'm rendering a div inside the main container to render the background

Testing Instructions

  1. Create a new post
  2. Add a cover block
  3. Add an image
  4. Add duotone for the image
  5. Select the fixed background option
  6. Save and navigate the post

Screenshots or screencast

duotone2.mp4

@MaggieCabrera MaggieCabrera added the [Block] Cover Affects the Cover Block - used to display content laid over a background image label Apr 25, 2022
Copy link
Contributor

@ajlende ajlende left a comment

Choose a reason for hiding this comment

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

I'd suggest taking a look at the block deprecations documentation since we're updating the markup of the saved block

packages/block-library/src/cover/style.scss Show resolved Hide resolved
packages/block-library/src/cover/shared.js Outdated Show resolved Hide resolved
@matiasbenedetto
Copy link
Contributor Author

I'd suggest taking a look at the block deprecations documentation since we're updating the markup of the saved block

thanks @ajlende ! I tested the code with the block created before adding these changes so I'm not sure if we need a deprecation or not. What do you think?

@matiasbenedetto matiasbenedetto requested a review from ajlende April 29, 2022 14:40
@ajlende
Copy link
Contributor

ajlende commented May 10, 2022

I tested the code with the block created before adding these changes so I'm not sure if we need a deprecation or not. What do you think?

The general rule is that I had heard about adding block deprecations is to add one if the markup or attributes change from one block version to the next. I'd taken that at face value up until now and hadn't really questioned why, so I dug into the code a bit to get a better understanding of how deprecations work to see if I could create a test case that fails.

I managed to get it to break with this markup that I generated by following the testing instructions from #39145 and additionally including fixed+repeated backgrounds on the 6.0 beta.

<!-- wp:cover {"url":"https://cldup.com/Fz-ASbo2s3.jpg","hasParallax":true,"isRepeated":true,"dimRatio":50,"minHeightUnit":"em","isDark":false} -->
<div class="wp-block-cover is-light has-parallax is-repeated" style="background-image:url(https://cldup.com/Fz-ASbo2s3.jpg)"><span aria-hidden="true" class="wp-block-cover__background has-background-dim"></span><div class="wp-block-cover__inner-container"><!-- wp:paragraph {"align":"center","placeholder":"Write title…","fontSize":"large"} -->
<p class="has-text-align-center has-large-font-size"></p>
<!-- /wp:paragraph --></div></div>
<!-- /wp:cover -->

As mentioned in the documentation, block deprecations don't work like database migrations by running one after another to upgrade a block to a newer version. It basically just checks the block attributes against older versions of the block to confirm if the output was valid at one point in history. Then, if it finds a match, it saves the new output instead.

If you don't have minHeightUnit without a minHeight attribute in your test, the block deprecation matches just fine on the v9 deprecation and you don't see any issues.

I found that the code for running the block deprecations isn't too bad to step through in a debugger. I'd recommend setting a conditional breakpoint for rawBlock.name === 'core/cover' in parseRawBlock and/or in applyBlockDeprecatedVersions and then pasting the above code into the code editor if you want to see how it all works yourself.

@matiasbenedetto
Copy link
Contributor Author

matiasbenedetto commented May 10, 2022

Thanks, @ajlende very useful insight. I could reproduce the issue whit that markup and learned why that happen by inspecting the code you pointed out. I added a deprecation to handle the new markup.

@ajlende
Copy link
Contributor

ajlende commented May 10, 2022

Could you add a couple fixtures for the new block deprecation? Details in the docs for how to do that.

@matiasbenedetto
Copy link
Contributor Author

fixtures added! :)

@ajlende ajlende added [Type] Bug An existing feature does not function as intended [Package] Block library /packages/block-library labels May 10, 2022
Copy link
Contributor

@scruffian scruffian left a comment

Choose a reason for hiding this comment

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

LGTM, you just need to rebase, and I'll merge!

@matiasbenedetto
Copy link
Contributor Author

matiasbenedetto commented May 23, 2022

@scruffian @ajlende conflicts fixed :)

@ajlende ajlende merged commit 81dc943 into WordPress:trunk May 24, 2022
@github-actions github-actions bot added this to the Gutenberg 13.4 milestone May 24, 2022
@youknowriad youknowriad added the Needs Dev Note Requires a developer note for a major WordPress release cycle label May 24, 2022
@youknowriad
Copy link
Contributor

I added the dev note label because this one seems to change the markup of the cover block slightly (right?) just to tell theme authors about potential impact. If you think that's not needed, feel free to remove the label.

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 Needs Dev Note Requires a developer note for a major WordPress release cycle [Package] Block library /packages/block-library [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Duotone doesn't work with fixed background
5 participants