Gallery: re-enable block spacing at block level while still hiding in global styles #53900
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…y moving check to hide controls to global styles screen
andrewserong
added
[Type] Bug
andrewserong
requested review from
youknowriad,
glendaviesnz,
tellthemachines and
aaronrobertshaw
|
Size Change: +45 B (0%) Total Size: 1.51 MB
ℹ️ View Unchanged
|
aaronrobertshaw
approved these changes
Aug 24, 2023
There was a problem hiding this comment.
Thanks for the speedy fix on this @andrewserong 🚀
It tests as advertised for me and the changes look good.
With a block theme active in the Post Editor:
✅ Block gap controls show for gallery, columns, buttons etc.
With a block theme active in the Site Editor:
✅ Gap controls display for gallery, columns, buttons etc. in the block inspector
✅ No gap controls present for gallery blocks in Global Styles
✅ Gap controls still display for columns, buttons etc in Global Styles
With a classic theme active:
✅ No gap controls show in the post editor for any of the blocks tested
youknowriad
reviewed
Aug 24, 2023
| @@ -98,6 +98,20 @@ function ScreenBlock( { name, variation } ) { | |||
| const { behavior } = useGlobalBehaviors( name, 'user' ); | |||
|
|
|||
| const blockType = getBlockType( name ); | |||
|
|
|||
| // Only allow `blockGap` support if serialization has not been skipped, to be sure global spacing can be applied. | |||
There was a problem hiding this comment.
I find it a bit weird that we allow something globally but not locally if __experimentalSkipSerialization is true. That flag indicates that we can't serialize so why serialize for global styles but not serialize for local styles?
Assuming it's ok to do that, why are we doing this only for "block gap", it's probably not the only property that can have "__experimentalSkipSerialization".
There was a problem hiding this comment.
I find it a bit weird that we allow something globally but not locally if __experimentalSkipSerialization is true. That flag indicates that we can't serialize so why serialize for global styles but not serialize for local styles?
It is a weird check this one. The comment might be slightly ambiguous, but the logic is the inverse — for blockGap if __experimentalSkipSerialization is true then we don't allow global styles to be set. From memory, the reason this logic was introduced is because of the unique case of the Gallery block where we can't (yet) allow block level global styles for it since that block always outputs block-level styles even if a local blockGap value isn't set.
Assuming it's ok to do that, why are we doing this only for "block gap", it's probably not the only property that can have "__experimentalSkipSerialization".
My longer-term preference would be for us to actually remove this edge case for block gap entirely, so __experimentalSkipSerialization wouldn't disable block-level global styles. To do so, I believe we'd need to update the post editor to include a global styles provider so that at the block level for the Gallery block we can check if a block-level global styles value exists before outputting the local block-level Gallery styles.
I'll merge this PR in now, but happy to look into a proof of concept of how we might move away from this rule — it'll probably make the conversation a little clearer if we have some code to play with to look at an alternative 🙂
There was a problem hiding this comment.
It is a weird check this one. The comment might be slightly ambiguous, but the logic is the inverse — for blockGap if __experimentalSkipSerialization is true then we don't allow global styles to be set
Actually, that makes more sense. If it's the block author that provides the serialization, it makes sense to disable on global styles because I believe we have no way to define the "serialization" function in global styles output.
So maybe the best path forward here is to make this a generic rule for all block supports.
|
Thanks for reviewing @aaronrobertshaw and for the feedback @youknowriad! I'll merge this in now since it resolves the main pressing issue. Separately, I'll look into a proof of concept follow-up next week to explore how we might enable global styles for the Gallery block, and potentially look at removing this rule altogether (assuming I can get it working 😄) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Follow-up to #53008
Move logic to hide block spacing controls when
__experimentalSkipSerializationis in use to the block screen in global styles.This PR resolves an issue that possibly dates back to #48070, but was exposed in #53008.
Why?
The logic to hide the block spacing controls when
__experimentalSkipSerializationis in use was only intended to be applied to global styles screens. Prior to this PR, the check runs in all cases, and with #53008, it meant that at the block level,spacing.blockGapwas set tofalsefor the Gallery block, when it should have been enabled.An alternative to this PR would be to enable block spacing in global styles for the Gallery block. Unfortunately, I don't think we can quite do that yet. I hacked around a little and found that do that, we'd need to:
wp_get_global_style, and checking to see if a blockGap value is set for the gallery block)useGlobalStyle, because the post editor does not yet have a global styles provider, we don't have access to the styles values)So, in the shorter-term, I think the fix in this PR is likely the pragmatic way to go as it preserves the existing expected logic, but moves it to where it should be applied (by mutating
settingsto pass to theDimensionsPanelin the block screen in global styles). Once we have access to global styles values within the post editor, then in a follow-up, we could switch off the logic in this PR and enable Gallery block global styles.How?
Move logic to hide the block spacing controls if a block has
__experimentalSkipSerializationset to the block screen in global styles.Testing Instructions
Testing Instructions for Keyboard
Screenshots or screencast