-
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
Fix reusable block cancelling #5723
Conversation
Pressing Cancel while editing a reusable block should discard any unsaved changes that have been made to that block.
blocks/library/block/index.js
Outdated
@@ -30,6 +30,7 @@ class ReusableBlockEdit extends Component { | |||
this.state = { | |||
isEditing: !! ( reusableBlock && reusableBlock.isTemporary ), | |||
title: null, | |||
attributes: null, |
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.
I think the name of the state property could be improved to better indicate that it's the transient updates which have yet to be saved. Otherwise it's a bit difficult to discern the difference between this.state.attributes
and this.props.block.attributes
.
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.
👌 fixed in 5a36e93.
blocks/library/block/index.js
Outdated
if ( prevState.attributes !== null ) { | ||
return { attributes: { ...prevState.attributes, ...attributes } }; | ||
} | ||
return null; |
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.
Is an explicit return value necessary here? Or could we drop this line altogether?
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.
👌 fixed in 5a36e93.
blocks/library/block/index.js
Outdated
|
||
this.setState( { | ||
isEditing: true, | ||
title: reusableBlock.title, | ||
attributes: block.attributes, |
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.
Do we need to track all of the block's original attributes in state? Or could we simply track the state as the object which assigns into block.attributes
as the base?
this.setState( {
attributes: {},
} );
// ...
const attributes = { ...block.attributes, ...this.state.attributes };
Also has the added advantage that when we call updateAttributes
in save
, we're only applying the patch changes to state (likely to save a couple iterations when aggregating into state value and maybe avoiding some false positives when overriding state value for undo).
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.
👌 fixed in 5a36e93.
Avoid storing all of the referenced block's attributes in state by storing only the changed attributes, and name this state property changedAttributes to reflect this and to avoid ambiguity.
Makes it so that pressing Cancel while editing a reusable block discards any unsaved changes that have been made to that block.
This fixes #5719, which is a regression introduced by #5228.
To test: