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

Block Editor: Make initial inner blocks non-dirtying. #19521

Merged
merged 2 commits into from
Jan 17, 2020
Merged

Block Editor: Make initial inner blocks non-dirtying. #19521

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
f090278
Block Editor: Make initial inner blocks non-dirtying.
epiqueras Jan 9, 2020
393a7c9
Block Editor: Clear `markNextChangeAsNotPersistent` flag.
epiqueras Jan 17, 2020
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
4 changes: 4 additions & 0 deletions packages/block-editor/src/components/inner-blocks/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ class InnerBlocks extends Component {
templateLock,
__experimentalBlocks,
replaceInnerBlocks,
__unstableMarkNextChangeAsNotPersistent,
} = this.props;
const { innerBlocks } = block;
// Only synchronize innerBlocks with template if innerBlocks are empty or a locking all exists directly on the block.
Expand All @@ -56,6 +57,7 @@ class InnerBlocks extends Component {

// Set controlled blocks value from parent, if any.
if ( __experimentalBlocks ) {
__unstableMarkNextChangeAsNotPersistent();
replaceInnerBlocks( __experimentalBlocks );
}
}
Expand Down Expand Up @@ -184,6 +186,7 @@ InnerBlocks = compose( [
withDispatch( ( dispatch, ownProps ) => {
const {
replaceInnerBlocks,
__unstableMarkNextChangeAsNotPersistent,
updateBlockListSettings,
} = dispatch( 'core/block-editor' );
const { block, clientId, templateInsertUpdatesSelection = true } = ownProps;
Expand All @@ -198,6 +201,7 @@ InnerBlocks = compose( [
blocks.length !== 0
);
},
__unstableMarkNextChangeAsNotPersistent,
updateNestedSettings( settings ) {
dispatch( updateBlockListSettings( clientId, settings ) );
},
Expand Down
9 changes: 9 additions & 0 deletions packages/block-editor/src/store/actions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -811,6 +811,15 @@ export function __unstableMarkLastChangeAsPersistent() {
return { type: 'MARK_LAST_CHANGE_AS_PERSISTENT' };
}

/**
* Returns an action object used in signalling that the next block change should be marked explicitly as not persistent.
*
* @return {Object} Action object.
*/
export function __unstableMarkNextChangeAsNotPersistent() {
return { type: 'MARK_NEXT_CHANGE_AS_NOT_PERSISTENT' };
Copy link
Member

Choose a reason for hiding this comment

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

To mark a change as persistent, we use MARK_LAST_CHANGE_AS_PERSISTENT to mark the previous change as persistent, and we trow the action after the action we want to mark. While to explicitly mark a change as not persistent, we use MARK_NEXT_CHANGE_AS_NOT_PERSISTENT, and we trow it before the action we want to mark.

Would it be possible to make the usage of the actions more similar and trow both actions before or after the actions we want to mark?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, because of usage nuance:

In our framework, when you want a persistent change to not be persistent, it's so that the non-dirtying change handler is called from the block editor. If you fire an edit, and then fire a MARK_NEXT_CHANGE_AS_NOT_PERSISTENT, nothing guarantees that the wrong change handler wasn't already called before the MARK_NEXT_CHANGE_AS_NOT_PERSISTENT.

MARK_LAST_CHANGE_AS_PERSISTENT is different, because when using it, it's ok that the non-dirtying handler might have already been called, it will just call the dirtying one again with the same values.

}

/**
* Returns an action object used in signalling that the last block change is
* an automatic change, meaning it was not performed by the user, and can be
Expand Down
13 changes: 8 additions & 5 deletions packages/block-editor/src/store/reducer.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -350,15 +350,18 @@ const withBlockCache = ( reducer ) => ( state = {}, action ) => {
*/
function withPersistentBlockChange( reducer ) {
let lastAction;
let markNextChangeAsNotPersistent = false;
Copy link
Member

Choose a reason for hiding this comment

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

Should we name this with the "is" prefix? Something like isNextActionExplicitlyNotPersistent?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That is harder to read.


return ( state, action ) => {
let nextState = reducer( state, action );

const isExplicitPersistentChange = action.type === 'MARK_LAST_CHANGE_AS_PERSISTENT';
const isExplicitPersistentChange = action.type === 'MARK_LAST_CHANGE_AS_PERSISTENT' || markNextChangeAsNotPersistent;
Copy link
Member

Choose a reason for hiding this comment

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

This flag seems a little bit confusing because it is true if the change is explicitly persistent or explicitly not persistent, right? The name seems to indicate the flag is true only when the change is explicit persistent, but not when the change is explicitly not persistent.
I guess we could keep the flag as it was before and do them or when we need something explicitly persistent or explicitly not persistent? Or name the flag something like isExplicitPersistentOrNotChange ? (not a very good name but is more detailed).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The name seems to indicate the flag is true only when the change is explicit persistent, but not when the change is explicitly not persistent.

It's an "explicit persistent change". The change could be to true or false.


// Defer to previous state value (or default) unless changing or
// explicitly marking as persistent.
if ( state === nextState && ! isExplicitPersistentChange ) {
markNextChangeAsNotPersistent = action.type === 'MARK_NEXT_CHANGE_AS_NOT_PERSISTENT';

const nextIsPersistentChange = get( state, [ 'isPersistentChange' ], true );
if ( state.isPersistentChange === nextIsPersistentChange ) {
return state;
Expand All @@ -372,16 +375,16 @@ function withPersistentBlockChange( reducer ) {

nextState = {
...nextState,
isPersistentChange: (
isExplicitPersistentChange ||
! isUpdatingSameBlockAttribute( action, lastAction )
),
isPersistentChange: isExplicitPersistentChange ?
! markNextChangeAsNotPersistent :
! isUpdatingSameBlockAttribute( action, lastAction ),
};

// In comparing against the previous action, consider only those which
// would have qualified as one which would have been ignored or not
// have resulted in a changed state.
lastAction = action;
markNextChangeAsNotPersistent = action.type === 'MARK_NEXT_CHANGE_AS_NOT_PERSISTENT';

return nextState;
};
Expand Down