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.

Sign up for GitHub

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

Jump to bottom

Gallery: re-enable block spacing at block level while still hiding in global styles #53900

Merged
merged 1 commit into from
Aug 24, 2023
Merged

Gallery: re-enable block spacing at block level while still hiding in global styles #53900

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 1 addition & 9 deletions packages/blocks/src/store/private-selectors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,15 +106,7 @@ export const getSupportedStyles = createSelector(

// Check for blockGap support.
// Block spacing support doesn't map directly to a single style property, so needs to be handled separately.
// Also, only allow `blockGap` support if serialization has not been skipped, to be sure global spacing can be applied.
if (
blockType?.supports?.spacing?.blockGap &&
blockType?.supports?.spacing?.__experimentalSkipSerialization !==
true &&
! blockType?.supports?.spacing?.__experimentalSkipSerialization?.some?.(
( spacingType ) => spacingType === 'blockGap'
)
) {
if ( blockType?.supports?.spacing?.blockGap ) {
supportKeys.push( 'blockGap' );
}

Expand Down
14 changes: 14 additions & 0 deletions packages/edit-site/src/components/global-styles/screen-block.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.
Copy link
Contributor

Choose a reason for hiding this comment

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

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".

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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 🙂

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

if (
settings?.spacing?.blockGap &&
blockType?.supports?.spacing?.blockGap &&
( blockType?.supports?.spacing?.__experimentalSkipSerialization ===
true ||
blockType?.supports?.spacing?.__experimentalSkipSerialization?.some?.(
( spacingType ) => spacingType === 'blockGap'
) )
) {
settings.spacing.blockGap = false;
}

const blockVariations = useBlockVariations( name );
const hasTypographyPanel = useHasTypographyPanel( settings );
const hasColorPanel = useHasColorPanel( settings );
Expand Down
Loading