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: Prevent transform to Group block when featured image is set. #42638

Conversation

ndiego
Copy link
Member

@ndiego ndiego commented Jul 22, 2022

What?

Partially fixes #42613

Why?

Logic was added to the Cover block (#40293, #40212) that allows it to transform into a Group block in certain circumstances. While this functionality might be debatable, it's disabled when the Cover block has media applied. The original PRs did not include a check for the new Featured Image option. This PR corrects for that.

How?

We simply check if the useFeaturedImage attribute is set. If so, we ignore the custom Cover --> Group transform and use the default Group transform.

Testing Instructions

  1. Add a Cover block to a post of page that have a Featured Image
  2. Choose Featured Image as the media option on the Cover block.
  3. Group the Cover block and see that the Cover block is now placed inside of a Group block. Previously the Cover block would have been transformed into a Group block and the Featured Image lost.

Screenshots or screencast

Functionality prior to this PR:
cover-block-before

Functionality after this PR:
cover-block-after

@ndiego ndiego 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 Jul 22, 2022
@ndiego ndiego requested a review from ajitbohra as a code owner July 22, 2022 16:26
@ndiego ndiego self-assigned this Jul 22, 2022
Copy link
Contributor

@andrewserong andrewserong left a comment

Choose a reason for hiding this comment

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

Thanks for this PR @ndiego, I think this is a good pragmatic fix for the issue described and works well for me.

I think perhaps there's a slight difference for user expectations between transforming the Cover block to Group and "grouping" a cover block, which would be good for us to explore separately (I'll add a comment on #42613).

This looks good to go to me, though!

@ndiego
Copy link
Member Author

ndiego commented Jul 25, 2022

Thanks for reviewing @andrewserong. I added an additional comment on #42613 as well.

As far as this PR, I keep getting this e2e failure related to not having permissions to create Navigation Menus?! It looks like a flaky test and is not relevant to this PR. Any ideas? I can try rebasing 🤔

@andrewserong
Copy link
Contributor

It looks like a flaky test and is not relevant to this PR. Any ideas

I saw quite a few test failures related to Navigation last week, and eventually re-running the tests got it passing, so my hunch is that it's unrelated to this PR, too. 🤞 re-running and/or rebasing might get them passing?

@ndiego
Copy link
Member Author

ndiego commented Jul 25, 2022

Rerunning the tests worked, thanks @andrewserong. 🚢 ing

@ndiego ndiego merged commit 7fd28e4 into WordPress:trunk Jul 25, 2022
@ndiego ndiego deleted the fix/prevent-cover-with-media-converting-to-group branch July 25, 2022 11:17
@github-actions github-actions bot added this to the Gutenberg 13.8 milestone Jul 25, 2022
@ndiego ndiego added the Backport to WP Minor Release Pull request that needs to be backported to a WordPress minor release label Jul 26, 2022
@gziolo
Copy link
Member

gziolo commented Aug 23, 2022

I checked #40293, and it looks like the updated transform was added after WordPress 6.0 Beta 1. There is nothing to do for WordPress 6.0.2 as the code doesn't exist in the wp/6.0 branch.

@gziolo gziolo removed the Backport to WP Minor Release Pull request that needs to be backported to a WordPress minor release label Aug 23, 2022
@femkreations femkreations added the Needs User Documentation Needs new user documentation label Sep 3, 2022
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 User Documentation Needs new user documentation [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Group contextual menu option: Ensure Cover block is wrapped in a Group block
4 participants