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

Shadow support enable skip serialization for dynamic blocks #59887

Conversation

colinduwe
Copy link
Contributor

What?

When a dynamic block defines __experimentalSkipSerialization in its block.supports.shadow the styles continue to be printed to the block wrapper element. This PR corrects that behavior so that shadow behaves like border, color, and others.

Why?

This provides the expected behavior for dynamic blocks that need to opt out of having the shadow styles printed to the block wrapper.

How?

Added a check at the start of the render function to return an empty array of the block has set __experimentalSkipSerialization

Testing Instructions

  1. Choose a core dynamic block such as core/post-featured-image or create a dynamic block for testing
  2. Add shadow to the block supports in block.json
  3. Build the block
  4. Add the block in a post and verify that the shadow styles are printed on the block wrapper
  5. Update shadow to include "__experimentalSkipSerialization": true
  6. Rebuild the block
  7. Update the post and verify that the shadow styles are not printed on the block wrapper

There is a risk of a deprecation/regression so also check the core blocks that already support shadow

  1. core/button
  2. core/column
  3. core/columns
  4. core/image
    Core/image uses __experimentalSkipSerialization so verify that there is no change in the editor or front end with this PR.
    These are not dynamic blocks so should be unaffected by this change.

@colinduwe colinduwe requested a review from spacedmonkey as a code owner March 14, 2024 20:50
Copy link

The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the props-bot label.

If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.

Co-authored-by: colinduwe <colind@git.wordpress.org>

To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook.

@github-actions github-actions bot added the First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository label Mar 14, 2024
Copy link

👋 Thanks for your first Pull Request and for helping build the future of Gutenberg and WordPress, @colinduwe! In case you missed it, we'd love to have you join us in our Slack community.

If you want to learn more about WordPress development in general, check out the Core Handbook full of helpful information.

@colinduwe
Copy link
Contributor Author

@aaronrobertshaw Here's a PR for improving the /supports/shadow as discussed at #59616

@aaronrobertshaw aaronrobertshaw added [Type] Bug An existing feature does not function as intended [Feature] Design Tools Tools that impact the appearance of blocks both to expand the number of tools and improve the experi Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta labels Mar 15, 2024
Copy link
Contributor

@aaronrobertshaw aaronrobertshaw 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 splitting this out @colinduwe 👍

This is testing well for me. I used the dynamic Cover and Post Featured Image blocks for the majority of testing.

✅ Code change looks good
✅ Dynamic blocks that don't skip serialization still get styles on their wrapper
✅ Dynamic blocks that do skip serialization no longer get styles on the wrapper
✅ Static blocks continue to function correctly

LGTM 🚢

I'll merge this PR for you shortly.

The next step is to backport this PHP change to WordPress Core to be included in the WP 6.5 release. Is that something that you'd like to do?

There's some documentation available on how to do that and you can ping me in case you need a little help.

@aaronrobertshaw aaronrobertshaw merged commit 63f95db into WordPress:trunk Mar 15, 2024
62 of 63 checks passed
@github-actions github-actions bot added this to the Gutenberg 18.0 milestone Mar 15, 2024
@colinduwe
Copy link
Contributor Author

Thanks @aaronrobertshaw! Here's an attempt at the backport https://core.trac.wordpress.org/ticket/60784#ticket

@youknowriad
Copy link
Contributor

This seems like an enhancement (add support for skipping serialization) to a new block support. So not really a regression or bug fix, so I'm removing the backport label (this needs to land in 6.6)

@youknowriad youknowriad added Needs PHP backport Needs PHP backport to Core and removed Backport to WP 6.7 Beta/RC Pull request that needs to be backported to the WordPress major release that's currently in beta labels Mar 18, 2024
@aaronrobertshaw
Copy link
Contributor

Happy to go with your call on this one 👍

I still see this as a bug given the support for skipping serialization was added in #58306 which lands in GB 17.7 & WP 6.5. If a consumer tries to use that for a dynamic block, as the block.json schema indicates is available, it won't work without this fix.

That said, I appreciate it can be argued that "support for dynamic blocks" is the enhancement.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Design Tools Tools that impact the appearance of blocks both to expand the number of tools and improve the experi First-time Contributor Pull request opened by a first-time contributor to Gutenberg repository Needs PHP backport Needs PHP backport to Core [Package] Block editor /packages/block-editor [Status] In Progress Tracking issues with work in progress [Type] Bug An existing feature does not function as intended
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants