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

PostPublishButton: Disable if saving non-post entities #33140

Merged
merged 8 commits into from
Jul 8, 2021
Merged

PostPublishButton: Disable if saving non-post entities #33140

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
a085c04
core-data: Add __experimentalGetEntitiesBeingSaved() selector
ockham Jul 1, 2021
c979ca3
Editor: Add isSavingNonPostEntityChanges() selector
ockham Jul 1, 2021
8245031
PostPublishButton: Use isSavingNonPostEntityChanges() to determine bu…
ockham Jul 1, 2021
50e896f
Disable the Cancel button, too
ockham Jul 1, 2021
dc72ccb
Add unit test for __experimentalGetEntitiesBeingSaved
ockham Jul 5, 2021
ff1ca28
Add unit test coverage for isSavingNonPostEntityChanges
ockham Jul 5, 2021
73c59c2
Typo
ockham Jul 5, 2021
61d0634
Absorb non-post entity related criteria into is{Button|Toggle}Disabled
ockham Jul 7, 2021
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
50 changes: 50 additions & 0 deletions packages/core-data/src/selectors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -316,6 +316,56 @@ export const __experimentalGetDirtyEntityRecords = createSelector(
( state ) => [ state.entities.data ]
);

/**
* Returns the list of entities currently being saved.
*
* @param {Object} state State tree.
*
* @return {[{ title: string, key: string, name: string, kind: string }]} The list of records being saved.
*/
export const __experimentalGetEntitiesBeingSaved = createSelector(
( state ) => {
const {
entities: { data },
} = state;
const recordsBeingSaved = [];
Object.keys( data ).forEach( ( kind ) => {
Object.keys( data[ kind ] ).forEach( ( name ) => {
const primaryKeys = Object.keys(
data[ kind ][ name ].saving
).filter( ( primaryKey ) =>
isSavingEntityRecord( state, kind, name, primaryKey )
);

if ( primaryKeys.length ) {
const entity = getEntity( state, kind, name );
primaryKeys.forEach( ( primaryKey ) => {
const entityRecord = getEditedEntityRecord(
state,
kind,
name,
primaryKey
);
recordsBeingSaved.push( {
// We avoid using primaryKey because it's transformed into a string
// when it's used as an object key.
key:
entityRecord[
entity.key || DEFAULT_ENTITY_KEY
],
title: entity?.getTitle?.( entityRecord ) || '',
name,
kind,
} );
} );
}
ockham marked this conversation as resolved.
Show resolved Hide resolved
} );
} );
return recordsBeingSaved;
},
( state ) => [ state.entities.data ]
);

/**
* Returns the specified entity record's edits.
*
Expand Down
49 changes: 49 additions & 0 deletions packages/core-data/src/test/selectors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import {
hasEntityRecords,
getEntityRecords,
__experimentalGetDirtyEntityRecords,
__experimentalGetEntitiesBeingSaved,
getEntityRecordNonTransientEdits,
getEmbedPreview,
isPreviewEmbedFallback,
Expand Down Expand Up @@ -396,6 +397,54 @@ describe( '__experimentalGetDirtyEntityRecords', () => {
} );
} );

describe( '__experimentalGetEntitiesBeingSaved', () => {
it( "should return a map of objects with each raw entity record that's being saved", () => {
const state = deepFreeze( {
entities: {
config: [
{
kind: 'someKind',
name: 'someName',
transientEdits: { someTransientEditProperty: true },
},
],
data: {
someKind: {
someName: {
queriedData: {
items: {
default: {
someKey: {
someProperty: 'somePersistedValue',
someRawProperty: {
raw: 'somePersistedRawValue',
},
id: 'someKey',
},
},
},
itemIsComplete: {
default: {
someKey: true,
},
},
},
saving: {
someKey: {
pending: true,
},
},
},
},
},
},
} );
expect( __experimentalGetEntitiesBeingSaved( state ) ).toEqual( [
{ kind: 'someKind', name: 'someName', key: 'someKey', title: '' },
] );
} );
} );

describe( 'getEntityRecordNonTransientEdits', () => {
it( 'should return an empty object when the entity does not have a loaded config.', () => {
const state = deepFreeze( {
Expand Down
29 changes: 17 additions & 12 deletions packages/editor/src/components/post-publish-button/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,21 +103,24 @@ export class PostPublishButton extends Component {
onToggle,
visibility,
hasNonPostEntityChanges,
isSavingNonPostEntityChanges,
} = this.props;

const isButtonDisabled =
isSaving ||
forceIsSaving ||
! isSaveable ||
isPostSavingLocked ||
( ! isPublishable && ! forceIsDirty );
( isSaving ||
forceIsSaving ||
! isSaveable ||
isPostSavingLocked ||
( ! isPublishable && ! forceIsDirty ) ) &&
( ! hasNonPostEntityChanges || isSavingNonPostEntityChanges );

const isToggleDisabled =
isPublished ||
isSaving ||
forceIsSaving ||
! isSaveable ||
( ! isPublishable && ! forceIsDirty );
( isPublished ||
isSaving ||
forceIsSaving ||
! isSaveable ||
( ! isPublishable && ! forceIsDirty ) ) &&
( ! hasNonPostEntityChanges || isSavingNonPostEntityChanges );

let publishStatus;
if ( ! hasPublishAction ) {
Expand Down Expand Up @@ -147,15 +150,15 @@ export class PostPublishButton extends Component {
};

const buttonProps = {
'aria-disabled': isButtonDisabled && ! hasNonPostEntityChanges,
'aria-disabled': isButtonDisabled,
className: 'editor-post-publish-button',
isBusy: ! isAutoSaving && isSaving && isPublished,
variant: 'primary',
onClick: this.createOnClick( onClickButton ),
};

const toggleProps = {
'aria-disabled': isToggleDisabled && ! hasNonPostEntityChanges,
'aria-disabled': isToggleDisabled,
'aria-expanded': isOpen,
className: 'editor-post-publish-panel__toggle',
isBusy: isSaving && isPublished,
Expand Down Expand Up @@ -210,6 +213,7 @@ export default compose( [
getCurrentPostType,
getCurrentPostId,
hasNonPostEntityChanges,
isSavingNonPostEntityChanges,
} = select( editorStore );
const _isAutoSaving = isAutosavingPost();
return {
Expand All @@ -229,6 +233,7 @@ export default compose( [
postType: getCurrentPostType(),
postId: getCurrentPostId(),
hasNonPostEntityChanges: hasNonPostEntityChanges(),
isSavingNonPostEntityChanges: isSavingNonPostEntityChanges(),
};
} ),
withDispatch( ( dispatch ) => {
Expand Down
9 changes: 8 additions & 1 deletion packages/editor/src/components/post-publish-panel/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,6 +62,7 @@ export class PostPublishPanel extends Component {
isPublishSidebarEnabled,
isScheduled,
isSaving,
isSavingNonPostEntityChanges,
onClose,
onTogglePublishSidebar,
PostPublishExtension,
Expand Down Expand Up @@ -97,7 +98,11 @@ export class PostPublishPanel extends Component {
/>
</div>
<div className="editor-post-publish-panel__header-cancel-button">
<Button onClick={ onClose } variant="secondary">
<Button
disabled={ isSavingNonPostEntityChanges }
onClick={ onClose }
variant="secondary"
>
{ __( 'Cancel' ) }
</Button>
</div>
Expand Down Expand Up @@ -140,6 +145,7 @@ export default compose( [
isEditedPostBeingScheduled,
isEditedPostDirty,
isSavingPost,
isSavingNonPostEntityChanges,
} = select( editorStore );
const { isPublishSidebarEnabled } = select( editorStore );
const postType = getPostType( getEditedPostAttribute( 'type' ) );
Expand All @@ -156,6 +162,7 @@ export default compose( [
isPublished: isCurrentPostPublished(),
isPublishSidebarEnabled: isPublishSidebarEnabled(),
isSaving: isSavingPost(),
isSavingNonPostEntityChanges: isSavingNonPostEntityChanges(),
isScheduled: isCurrentPostScheduled(),
};
} ),
Expand Down
23 changes: 23 additions & 0 deletions packages/editor/src/store/selectors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -802,6 +802,29 @@ export const isSavingPost = createRegistrySelector( ( select ) => ( state ) => {
);
} );

/**
* Returns true if non-post entities are currently being saved, or false otherwise.
*
* @param {Object} state Global application state.
*
* @return {boolean} Whether non-post entities are being saved.
*/
export const isSavingNonPostEntityChanges = createRegistrySelector(
Copy link
Contributor

Choose a reason for hiding this comment

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

I think that selector name is a bit confusing, it doesn't match what it's actually doing. Because in theory, you could be saving other posts (think query block) that are different from the main one being edited in the page.

So it's more like

isSavingExtraEntities or isSavingMultipleEntities or something like that

Copy link
Contributor

Choose a reason for hiding this comment

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

Makes me wonder though whether we should just check that we're saving any entity including the "post" one. I mean do we need the distinction in the component where we call the selector? Maybe the selector could be simpler if we just check whether there's an entity save in progress (regardless of which entity it is)

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 think that selector name is a bit confusing, it doesn't match what it's actually doing. Because in theory, you could be saving other posts (think query block) that are different from the main one being edited in the page.

So it's more like

isSavingExtraEntities or isSavingMultipleEntities or something like that

That's true, but I modeled isSavingNonPostEntityChanges pretty much after hasNonPostEntityChanges, and the same points could be made about that selector, no? So I thought it'd make sense to name the "is-saving" counterpart to the "has...changes" selector similarly...

gutenberg/packages/editor/src/store/selectors.js

Lines 150 to 172 in 0216850

/**
* Returns true if there are unsaved edits for entities other than
* the editor's post, and false otherwise.
*
* @param {Object} state Global application state.
*
* @return {boolean} Whether there are edits or not.
*/
export const hasNonPostEntityChanges = createRegistrySelector(
( select ) => ( state ) => {
const dirtyEntityRecords = select(
coreStore
).__experimentalGetDirtyEntityRecords();
const { type, id } = getCurrentPost( state );
return some(
dirtyEntityRecords,
( entityRecord ) =>
entityRecord.kind !== 'postType' ||
entityRecord.name !== type ||
entityRecord.key !== id
);
}
);

Copy link
Contributor

Choose a reason for hiding this comment

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

Any thoughts about my second comment here, it makes the first one irrelevant because maybe the selector is not needed at all

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Makes me wonder though whether we should just check that we're saving any entity including the "post" one. I mean do we need the distinction in the component where we call the selector?

It seems like in the context of the PostPublishButton, that would be fine (since it's all going into isButtonDisabled and isToggleDisabled anyway). About other contexts, I'm less sure. Even within the PostPublishPanel, we have such props as isSaving and isSaveable (connected to the isSavingPost() and isEditedPostSaveable() selectors, respectively), but also forceIsSaving 😖 So that's not something I'm eager to touch 😬

Now if we're going to keep isSavingPost() in a number of other contexts (at least for now), it does seem to make sense to me to have a selector that's complementary to it -- i.e. checks if we're saving changes to anything but the post that's currently being edited.

In the long run, it might make sense to replace isSavingPost() by a wholesale isSavingEntities() selector, but that'll require an audit of all callsites. For the time being, I think it would add to the confusion to use such a wholesale selector for "is-saving" detection, when OTOH we have a dedicated hasNonPostEntityChanges for "is-dirty" detection.

Maybe the selector could be simpler if we just check whether there's an entity save in progress (regardless of which entity it is)

FWIW, that would basically just be __experimentalGetEntitiesBeingSaved(), right? Which is the counterpart to __experimentalGetDirtyEntityRecords().

Copy link
Contributor

Choose a reason for hiding this comment

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

That makes sense to me yeah. I have a feeling that our saving flow UI bits are way too complex and very hard to maintain for what they really are meant to do. they are also very old components that didn't see a lot of improvements over time aside the addition of adhoc multi-entities saving flow related changes but without rethinking the components themselves . I do think we need to check there for simplification/refactoring opportunities soon.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Indeed! Coincidentally, here's a related UX issue: #33223

( select ) => ( state ) => {
const entitiesBeingSaved = select(
coreStore
).__experimentalGetEntitiesBeingSaved();
const { type, id } = getCurrentPost( state );
return some(
entitiesBeingSaved,
( entityRecord ) =>
entityRecord.kind !== 'postType' ||
entityRecord.name !== type ||
entityRecord.key !== id
);
ockham marked this conversation as resolved.
Show resolved Hide resolved
}
);

/**
* Returns true if a previous post save was attempted successfully, or false
* otherwise.
Expand Down
55 changes: 55 additions & 0 deletions packages/editor/src/store/test/selectors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,6 +40,13 @@ selectorNames.forEach( ( name ) => {
);
},

__experimentalGetEntitiesBeingSaved() {
return (
state.__experimentalGetEntitiesBeingSaved &&
state.__experimentalGetEntitiesBeingSaved()
);
},

getEntityRecordEdits() {
const present = state.editor && state.editor.present;
let edits = present && present.edits;
Expand Down Expand Up @@ -170,6 +177,7 @@ const {
getCurrentPostAttribute,
getEditedPostAttribute,
isSavingPost,
isSavingNonPostEntityChanges,
didPostSaveRequestSucceed,
didPostSaveRequestFail,
getSuggestedPostFormat,
Expand Down Expand Up @@ -2085,6 +2093,53 @@ describe( 'selectors', () => {
} );
} );

describe( 'isSavingNonPostEntityChanges', () => {
it( 'should return true if changes to an arbitrary entity are being saved', () => {
const state = {
currentPost: { id: 1, type: 'post' },
__experimentalGetEntitiesBeingSaved() {
return [
{ kind: 'someKind', name: 'someName', key: 'someKey' },
];
},
};
expect( isSavingNonPostEntityChanges( state ) ).toBe( true );
} );
it( 'should return false if the only changes being saved are for the current post', () => {
const state = {
currentPost: { id: 1, type: 'post' },
__experimentalGetEntitiesBeingSaved() {
return [ { kind: 'postType', name: 'post', key: 1 } ];
},
};
expect( isSavingNonPostEntityChanges( state ) ).toBe( false );
} );
it( 'should return true if changes to multiple posts are being saved', () => {
const state = {
currentPost: { id: 1, type: 'post' },
__experimentalGetEntitiesBeingSaved() {
return [
{ kind: 'postType', name: 'post', key: 1 },
{ kind: 'postType', name: 'post', key: 2 },
];
},
};
expect( isSavingNonPostEntityChanges( state ) ).toBe( true );
} );
it( 'should return true if changes to multiple posts of different post types are being saved', () => {
const state = {
currentPost: { id: 1, type: 'post' },
__experimentalGetEntitiesBeingSaved() {
return [
{ kind: 'postType', name: 'post', key: 1 },
{ kind: 'postType', name: 'wp_template', key: 1 },
];
},
};
expect( isSavingNonPostEntityChanges( state ) ).toBe( true );
} );
} );

describe( 'didPostSaveRequestSucceed', () => {
it( 'should return true if the post save request is successful', () => {
const state = {
Expand Down