Skip to content

Add a control to reset pattern overrides #57845

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

Merged
kevin940726 merged 4 commits into from
Jan 17, 2024
Merged

Add a control to reset pattern overrides #57845

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9462711
Add a control to reset pattern overrides
kevin940726 Jan 15, 2024
6b4c307
Address code reviews
kevin940726 Jan 16, 2024
ab97ea5
Don't render reset button when there is no binding blocks
kevin940726 Jan 16, 2024
5e93c42
Set the `__experimentalIsFocusable` prop on the Reset to original too…
talldan Jan 17, 2024
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
26 changes: 21 additions & 5 deletions packages/block-library/src/block/edit.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -208,15 +208,18 @@ export default function ReusableBlockEdit( {
[ innerBlocks, setBlockEditingMode ]
);

// Apply the initial overrides from the pattern block to the inner blocks.
useEffect( () => {
const initialBlocks =
const initialBlocks = useMemo(
() =>
// Clone the blocks to generate new client IDs.
editedRecord.blocks?.map( ( block ) => cloneBlock( block ) ) ??
( editedRecord.content && typeof editedRecord.content !== 'function'
? parse( editedRecord.content )
: [] );
: [] ),
[ editedRecord.blocks, editedRecord.content ]
);

// Apply the initial overrides from the pattern block to the inner blocks.
useEffect( () => {
defaultValuesRef.current = {};
const editingMode = getBlockEditingMode( patternClientId );
// Replace the contents of the blocks with the overrides.
Expand All @@ -237,7 +240,7 @@ export default function ReusableBlockEdit( {
}, [
__unstableMarkNextChangeAsNotPersistent,
patternClientId,
editedRecord,
initialBlocks,
replaceInnerBlocks,
registry,
getBlockEditingMode,
Expand Down Expand Up @@ -293,6 +296,10 @@ export default function ReusableBlockEdit( {
editOriginalProps.onClick( event );
};

const resetOverrides = () => {
replaceInnerBlocks( patternClientId, initialBlocks );
};

let children = null;

if ( hasAlreadyRendered ) {
Expand Down Expand Up @@ -333,6 +340,15 @@ export default function ReusableBlockEdit( {
</ToolbarGroup>
</BlockControls>
) }
{ overrides ? (
<BlockControls>
<ToolbarGroup>
<ToolbarButton onClick={ resetOverrides }>
{ __( 'Reset overrides' ) }
Copy link
Contributor

@talldan talldan Jan 16, 2024

Choose a reason for hiding this comment

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

A thought. I wonder if 'Reset to original' might be a slight improvement on the copy. It fits nicely with the 'Edit original' button next to it, and is slightly less technical.

</ToolbarButton>
</ToolbarGroup>
</BlockControls>
) : null }
Copy link
Contributor

@talldan talldan Jan 15, 2024

Choose a reason for hiding this comment

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

Actually, one thing is that selecting the button causes a focus loss due to the way it's no longer rendered, so it should probably have a aria-disabled state instead (one that is focusable).

Copy link
Member Author

@kevin940726 kevin940726 Jan 15, 2024

Choose a reason for hiding this comment

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

Good catch! Or should we move the focus to the pattern block instead? Not sure which is better atm.

Copy link
Contributor

@talldan talldan Jan 16, 2024

Choose a reason for hiding this comment

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

Most of the accessibility feedback I see is that focus shouldn't be moved unexpectedly if there's an alternative, so that's why I feel that a disabled state would be the way to go. There's also a lot of precedence for this in the block editor, like the undo/redo buttons that retain focus when they become disabled.

It does introduce the caveat around patterns without overrides, should they have a visible button or should it be disabled? My feeling is that there probably shouldn't be a button at all since it would never become enabled.

Copy link
Member Author

@kevin940726 kevin940726 Jan 16, 2024

Choose a reason for hiding this comment

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

The problem is that patterns without overrides have the same state as after resetting the overrides 🤔. Unless we keep the previous state somehow, but I think that's more confusing than always showing the disabled button. Am I misunderstanding something here?

Copy link
Contributor

@talldan talldan Jan 16, 2024

Choose a reason for hiding this comment

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

The problem is that patterns without overrides have the same state as after resetting the overrides

We do look through the inner blocks to check for any that can have overrides here:

gutenberg/packages/block-library/src/block/edit.js

Lines 141 to 148 in f6d29bd

function setBlockEditMode( setEditMode, blocks, mode ) {
blocks.forEach( ( block ) => {
const editMode =
mode || ( isPartiallySynced( block ) ? 'contentOnly' : 'disabled' );
setEditMode( block.clientId, editMode );
setBlockEditMode( setEditMode, block.innerBlocks, mode );
} );
}

So given we do that we could also derive whether the pattern has any override attributes within it at the same time and use that to control visibility of the button. I think you end up with three states:

  1. Pattern has inner blocks that can be overriden but has no override state - show the button but disabled
  2. Pattern has inner blocks that can be overriden and has override state - show the button enabled
  3. None of the pattern's inner blocks can be overriden - don't show the button

Copy link
Member Author

@kevin940726 kevin940726 Jan 16, 2024

Choose a reason for hiding this comment

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

Oh I see what you mean now! I was only considering the (1) and the (2) states.

{ children === null ? (
<div { ...innerBlocksProps } />
) : (
Expand Down