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

Pattern block: remove wrapping div #36090

Merged
merged 1 commit into from
Nov 2, 2021
Merged

Pattern block: remove wrapping div #36090

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
33 changes: 5 additions & 28 deletions packages/block-library/src/pattern/edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,11 +6,9 @@ import { useEffect } from '@wordpress/element';
import {
store as blockEditorStore,
useBlockProps,
useInnerBlocksProps,
} from '@wordpress/block-editor';
import { createBlock } from '@wordpress/blocks';

const PatternEdit = ( { attributes, clientId, isSelected } ) => {
const PatternEdit = ( { attributes, clientId } ) => {
const selectedPattern = useSelect(
( select ) =>
select( blockEditorStore ).__experimentalGetParsedPattern(
Expand All @@ -19,44 +17,23 @@ const PatternEdit = ( { attributes, clientId, isSelected } ) => {
[ attributes.slug ]
);

const hasSelection = useSelect(
( select ) =>
isSelected ||
select( blockEditorStore ).hasSelectedInnerBlock( clientId, true ),
[ isSelected, clientId ]
);

const {
replaceBlocks,
replaceInnerBlocks,
__unstableMarkNextChangeAsNotPersistent,
} = useDispatch( blockEditorStore );

// Run this effect when the block, or any of its InnerBlocks are selected.
// This replaces the Pattern block wrapper with a Group block.
// This ensures the markup structure and alignment are consistent between editor and view.
// This change won't be saved unless further changes are made to the InnerBlocks.
useEffect( () => {
if ( hasSelection && selectedPattern?.blocks ) {
__unstableMarkNextChangeAsNotPersistent();
replaceBlocks(
clientId,
createBlock( 'core/group', {}, selectedPattern.blocks )
);
}
}, [ hasSelection, selectedPattern?.blocks ] );

// Run this effect when the component loads.
// This adds the Pattern block template as InnerBlocks.
// This adds the Pattern's contents to the post.
// This change won't be saved.
// It will continue to pull from the pattern file unless changes are made to its respective template part.
useEffect( () => {
if ( selectedPattern?.blocks ) {
__unstableMarkNextChangeAsNotPersistent();
replaceInnerBlocks( clientId, selectedPattern.blocks );
replaceBlocks( clientId, selectedPattern.blocks );
}
}, [ selectedPattern?.blocks ] );

const props = useInnerBlocksProps( useBlockProps(), {} );
const props = useBlockProps();

return <div { ...props } />;
Copy link
Contributor

Choose a reason for hiding this comment

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

This works, it makes the "pattern block" a temporary block (only used to load the template to the editor) but there's a gotcha.

Previously, changes were only saved if the pattern itself was modified, now the pattern will stop being used as soon as the template (or template part) where it's used is modified.

I'm fine with the change if you're fine with this behavior.

Copy link
Contributor

Choose a reason for hiding this comment

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

Personally I would prefer to keep the previous behaviour as it keeps the templates more flexible. Could we use an invisible wrapper, like React.Fragment?

Copy link
Member

Choose a reason for hiding this comment

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

There's also the possibility of using a <div> with display: contents, although there's an accessibility bug with it in Safari at the moment that may make it a non-starter for the time being. https://caniuse.com/css-display-contents

Copy link
Contributor

Choose a reason for hiding this comment

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

Previously, changes were only saved if the pattern itself was modified, now the pattern will stop being used as soon as the template (or template part) where it's used is modified.

Generally, this seems fine to me. Given our current use cases at least, the translation has already been done at that point. Can someone think of a use case where that wouldn't be ok?

Copy link
Contributor

Choose a reason for hiding this comment

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

The use case would be that the user changes their site language after they edit a template. With this change, once a user selects a site language they will need to update their templates with new translations.

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 would prefer to keep the previous behaviour as it keeps the templates more flexible. Could we use an invisible wrapper, like React.Fragment?

I will try this.

Copy link
Contributor Author

@jffng jffng Nov 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I gave this a try and <Fragment> + inner blocks does not have the desired effect.

Previously, changes were only saved if the pattern itself was modified

I also tested a few more scenarios, and I don't think the templates are more that much more flexible with the previous behavior. Take the following example on trunk:

  • Inside the site editor, select the pattern or any of its inner blocks.
  • Make a separate change to the template (unrelated to the pattern)
  • Save

This will still cause the pattern to no longer from the pattern file.

With this PR, you can still make changes to other template parts, and the pattern will still pull from the file.

The use case would be that the user changes their site language after they edit a template.

How common is this? The problems of alignment introduced by adding this additional wrapper are significant, so would love to find a solution here that doesn't introduce it.

Copy link
Contributor

Choose a reason for hiding this comment

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

The use case would be that the user changes their site language after they edit a template.

Yeah, I think in general that use case seems like an edge case to me. As @jffng noted, the approach in this PR eliminates a somewhat prominent layout issue, so I'd be happy to settle for what's in this PR and consider anything beyond this behavior to be an enhancement or bug fix later on.

Copy link
Member

@ZebulanStanphill ZebulanStanphill Nov 1, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I agree that the approach in this PR is good enough for now and an improvement over master.

Also, I don't really know if it's applicable here, and it's certainly beyond the scope of this PR, but just to throw the idea out there: I wonder if we could find a solution via Shadow DOM? Browser support looks viable.

Copy link
Contributor

Choose a reason for hiding this comment

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

Happy to ship with this compromise for now

};
Expand Down
2 changes: 1 addition & 1 deletion packages/block-library/src/pattern/index.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ function render_block_core_pattern( $attributes ) {
}

$pattern = $registry->get_registered( $slug );
return do_blocks( '<div>' . $pattern['content'] . '</div>' );
return do_blocks( $pattern['content'] );
}

add_action( 'init', 'register_block_core_pattern' );