-
Notifications
You must be signed in to change notification settings - Fork 4.3k
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: Update shadow support to allow explicit skipping of serialization #58306
Conversation
Size Change: +2 B (0%) Total Size: 1.69 MB
ℹ️ View Unchanged
|
@madhusudhand and @vcanales, is it too late to switch
My understanding was that while the shadow block support had been added it hadn't been adopted by any blocks yet. Would that mean we have some flexibility here if we act quickly? |
I thought about this, but we're moving the Shadow controls out of the "Effects" panel and into the Border section —— #58298 (comment) — so I don't think the change would make sense at this point. |
Then in that case it really falls under the border supports, so it should be something like:
This would also then provide the option for the shadow controls to be a default (shown by default) or optional (must be toggled on from panel menu) control via I think the border support is due for stabilization as well but that is a separate job. |
Is the criteria for where the support key is defined dictated by where it is in the UI? I'm asking because to me, even if we group them together in the UI, Shadow and Border are complementary but different, so I'm not 100% convinced that shadow belongs inside the border supports. |
I wouldn't say that it is but it also seemed that where it was in the UI was part of the argument against placing under an It is probably important for it to be consistent with other supports though. It should:
Leaving it as a simple boolean flag, doesn't give much flexibility moving forward and potentially means hacky workarounds depending on other supports that give them less freedom to change as well. |
Agreed. Traveling back in history, it is introduced as top level property for the following reasons.
With recent feedback from @mtias, in future effects may host options such as transitions, animations etc.. and so we decided to move shadow out of effects. So, we kind of stuck shadow being top level property. To resolve, I am thinking of following two approaches. make the shadow support as an object and add a property called "enabled"
Or continue to use boolean for
|
Thanks for the additional history and context @madhusudhand 👍
Fair enough, regarding There's another option to those you outlined which is probably neater while still maintaining flexibility and allowing for skipping serialization, controlling default controls etc. The For example:
You can see some other examples where existing block supports check for a boolean I think this is important to nail down before it's used widely as once it's out in the wild we're stuck with supporting the shape released. |
0cd05a6
to
fb43c6e
Compare
Thank for the inputs @aaronrobertshaw. Updated the PR with suggested structure. |
eed03e7
to
b39841c
Compare
Thanks for iterating on this @madhusudhand 👍 I think it's working ok but it would help if we can update the PR description to match what the PR actually does now and include some test instructions. |
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 Core SVNIf you're a Core Committer, use this list when committing to
GitHub Merge commitsIf you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message.
To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
Updated the description and testing instructions. |
b39841c
to
994119a
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
✅ When serialization is skipped, no inline shadow style is applied to the block's wrapper
✅ When not skipped, the block wrapper gets the inline style
✅ Also tested by adding shadow support to a few other blocks
I hope you don't mind but I took the liberty to rebase the PR to resolve the conflicts with the core blocks doc. I also tweaked the PR title to be about allowing skipping serialization, as styles were already serialized.
What?
Shadow was introduced as a top level style property and enabled in block settings here. This is a followup PR to address the concerns related to style serialization.
Before this change, style serialization is skipped by default without additional
__experimentalSkipSerialization
attribute value, sinceshadow
support was always been a boolean.This change makes the
shadow
support either to be a boolean, or an object with the following structure.block.json
structureor
When the value of
__experimentalSkipSerialization
is set to true, block skips the style serialization.Testing instructions
block.json
of blocks such asbutton
with the above definitions.example templates:
when not skipped
when skipped