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

add minHeightUnit to latest core/cover deprecation #28627

Merged
merged 4 commits into from
Feb 1, 2021

Conversation

flootr
Copy link
Contributor

@flootr flootr commented Feb 1, 2021

Description

Add minHeightUnit attribute to latest deprecation of the core/cover block introduced in #25171 (part of Gutenberg 9.8).

This missing property resulted in invalidation of the Cover block when using custom units and upgrading from outdated markup.

Add tests for the regression.

Block validation: Block validation failed for `core/cover` ({name: "core/cover", icon: {…}, keywords: Array(0), attributes: {…}, providesContext: {…}, …}).

Content generated by `save` function:

<div class="wp-block-cover has-background-dim" style="min-height:71vw"><img class="wp-block-cover__image-background wp-image-7" alt="" src="[REDACTED]" style="object-position:61% 59%" data-object-fit="cover" data-object-position="61% 59%"/><div class="wp-block-cover__inner-container"></div></div>

Content retrieved from post body:

<div class="wp-block-cover has-background-dim" style="background-image:url([REDACTED]);min-height:71vw;background-position:61% 59%"><div class="wp-block-cover__inner-container"></div></div>

How has this been tested?

In Gutenberg 9.7.4 create a post and add a Cover block. Set a background image and change the minHeight unit from the block settings to something different than px (e.g. vw). Copy that block and paste it into Gutenberg 9.8.2. You should see invalidations in the browser console and a broken UI for the block.

With this patch applied you should see valid block UI and a message in the console that the block had been updated successfully.

The new test should pass.

Screenshots

Types of changes

Bug fix. minHeightUnit would otherwise be undefined.

Checklist:

  • My code is tested.
  • My code follows the WordPress code style.
  • My code follows the accessibility standards.
  • 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.

@github-actions github-actions bot added the First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository label Feb 1, 2021
@sirreal sirreal added [Block] Cover Affects the Cover Block - used to display content laid over a background image [Status] In Progress Tracking issues with work in progress [Type] Regression Related to a regression in the latest release labels Feb 1, 2021
@sirreal sirreal added this to the Gutenberg 9.8 milestone Feb 1, 2021
@sirreal
Copy link
Member

sirreal commented Feb 1, 2021

Will you add a CHANGELOG entry here?

@sirreal sirreal removed the [Status] In Progress Tracking issues with work in progress label Feb 1, 2021
Copy link
Member

@sirreal sirreal left a comment

Choose a reason for hiding this comment

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

I've tested by creating a Cover block with 9.7.4 then upgrading Gutenberg to 9.8.2. I see the invalidations as described.

Then I installed the build from this PR and loaded the post. I see the block upgrade messages as described.

👍 Thanks @flootr!

Let's just wait for the newly added tests to pass then this looks ready to me.

@ockham
Copy link
Contributor

ockham commented Feb 1, 2021

This needs a rebase in order to pick up the latest fixes to PHP unit tests (which I've just promoted to 'required' 😬 )

@sirreal
Copy link
Member

sirreal commented Feb 1, 2021

@ockham Rebased!

@ockham
Copy link
Contributor

ockham commented Feb 1, 2021

Thanks, this is looking good and appears to work well.

I think that it might make sense to update the existing packages/e2e-tests/fixtures/blocks/core__cover__deprecated-6.* files rather than introducing core__cover__deprecated-6-1.*, since we're only modifying an existing deprecation, rather than adding a new one 🤔

@sirreal
Copy link
Member

sirreal commented Feb 1, 2021

I think that it might make sense to update the existing packages/e2e-tests/fixtures/blocks/core__cover__deprecated-6.* files rather than introducing core__cover__deprecated-6-1.*, since we're only modifying an existing deprecation, rather than adding a new one 🤔

I merged the test into the same file. I considered replacing the test, but the reality is that deprecations need to be tested with different inputs. As we observed here, the deprecation was tested but didn't cover all the cases. It seems to make sense to maintain the existing test and add more as we need to test the deprecation's behavior on different serialized content.

@ockham
Copy link
Contributor

ockham commented Feb 1, 2021

E2e test failures (because of FSE related things) seem to be unrelated (and have been present for a while 😓 ). I'll merge, and will cherry-pick to both release/9.8 and release/9.9 (cc/ @nosolosw).

@ockham ockham merged commit 15712be into WordPress:master Feb 1, 2021
@github-actions
Copy link

github-actions bot commented Feb 1, 2021

Congratulations on your first merged pull request, @flootr! We'd like to credit you for your contribution in the post announcing the next WordPress release, but we can't find a WordPress.org profile associated with your GitHub account. When you have a moment, visit the following URL and click "link your GitHub account" under "GitHub Username" to link your accounts:

https://profiles.wordpress.org/me/profile/edit/

And if you don't have a WordPress.org account, you can create one on this page:

https://login.wordpress.org/register

Kudos!

ockham pushed a commit that referenced this pull request Feb 1, 2021
…HeightUnit` to latest core/cover deprecation (#28627)

Co-authored-by: Jon Surrell <jon.surrell@automattic.com> Co-authored-by: Jon Surrell <jon.surrell@automattic.com># Conflicts:
ockham pushed a commit that referenced this pull request Feb 1, 2021
Co-authored-by: Jon Surrell <jon.surrell@automattic.com>
@noisysocks noisysocks added the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Feb 8, 2021
noisysocks pushed a commit that referenced this pull request Feb 8, 2021
Co-authored-by: Jon Surrell <jon.surrell@automattic.com>
noisysocks pushed a commit that referenced this pull request Feb 8, 2021
Co-authored-by: Jon Surrell <jon.surrell@automattic.com>
@noisysocks noisysocks removed the Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta label Feb 8, 2021
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 First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository [Type] Regression Related to a regression in the latest release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants