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

Editor: Remove constants for notices #68361

Merged
merged 2 commits into from
Dec 30, 2024
Merged

Editor: Remove constants for notices #68361

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
7e2de0d
Editor: Remove constants for notices
Mamaduka Dec 27, 2024
425e33b
Use same ID for save notices
Mamaduka Dec 30, 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
2 changes: 0 additions & 2 deletions packages/editor/src/store/actions.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ import { __ } from '@wordpress/i18n';
/**
* Internal dependencies
*/
import { TRASH_POST_NOTICE_ID } from './constants';
import { localAutosaveSet } from './local-autosave';
import {
getNotificationArgumentsForSaveSuccess,
Expand Down Expand Up @@ -347,7 +346,6 @@ export const trashPost =
const postType = await registry
.resolveSelect( coreStore )
.getPostType( postTypeSlug );
registry.dispatch( noticesStore ).removeNotice( TRASH_POST_NOTICE_ID );
Copy link
Member Author

@Mamaduka Mamaduka Dec 27, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

There is no need to remove the failed trash notice. If actions fail again, the same notice will be reused because the notice has the same static ID. Users will be redirected if they succeed, and WP will display a different notice.

Copy link
Member

Choose a reason for hiding this comment

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

Makes sense to me 👍

const { rest_base: restBase, rest_namespace: restNamespace = 'wp/v2' } =
postType;
dispatch( { type: 'REQUEST_POST_DELETE_START' } );
Expand Down
2 changes: 0 additions & 2 deletions packages/editor/src/store/constants.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,6 @@ export const EDIT_MERGE_PROPERTIES = new Set( [ 'meta' ] );
*/
export const STORE_NAME = 'core/editor';

export const SAVE_POST_NOTICE_ID = 'SAVE_POST_NOTICE_ID';
export const TRASH_POST_NOTICE_ID = 'TRASH_POST_NOTICE_ID';
Comment on lines -14 to -15
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for working to clean those up 👏

export const PERMALINK_POSTNAME_REGEX = /%(?:postname|pagename)%/;
export const ONE_MINUTE_IN_MS = 60 * 1000;
export const AUTOSAVE_PROPERTIES = [ 'title', 'excerpt', 'content' ];
Expand Down
11 changes: 3 additions & 8 deletions packages/editor/src/store/utils/notice-builder.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,6 @@
*/
import { __ } from '@wordpress/i18n';

/**
* Internal dependencies
*/
import { SAVE_POST_NOTICE_ID, TRASH_POST_NOTICE_ID } from '../constants';

/**
* Builds the arguments for a success notification dispatch.
*
Expand Down Expand Up @@ -68,7 +63,7 @@ export function getNotificationArgumentsForSaveSuccess( data ) {
return [
noticeMessage,
{
id: SAVE_POST_NOTICE_ID,
id: 'editor-save',
type: 'snackbar',
actions,
},
Expand Down Expand Up @@ -113,7 +108,7 @@ export function getNotificationArgumentsForSaveFail( data ) {
return [
noticeMessage,
{
id: SAVE_POST_NOTICE_ID,
Copy link
Member

Choose a reason for hiding this comment

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

I think there was a reason that the save success and save error notices were using the same notice, though.

Right now if you do the following:

  • Save a post successfully
  • Quickly block the save REST API request URL
  • Save quickly, see failure notice

You'll see the success notice remains together with the failure one:

Screenshot 2024-12-30 at 13 10 31

Haven't tested, but I believe that if you keep the same ID for both error and success, the new (failure) notice should remove the old (success) one properly.

Copy link
Member Author

Choose a reason for hiding this comment

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

That's a good point. I'll update to use the same string value again.

id: 'editor-save',
},
];
}
Expand All @@ -131,7 +126,7 @@ export function getNotificationArgumentsForTrashFail( data ) {
? data.error.message
: __( 'Trashing failed' ),
{
id: TRASH_POST_NOTICE_ID,
id: 'editor-trash-fail',
},
];
}
7 changes: 3 additions & 4 deletions packages/editor/src/store/utils/test/notice-builder.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import {
getNotificationArgumentsForSaveFail,
getNotificationArgumentsForTrashFail,
} from '../notice-builder';
import { SAVE_POST_NOTICE_ID, TRASH_POST_NOTICE_ID } from '../../constants';

describe( 'getNotificationArgumentsForSaveSuccess()', () => {
const postType = {
Expand All @@ -27,7 +26,7 @@ describe( 'getNotificationArgumentsForSaveSuccess()', () => {
};
const post = { ...previousPost };
const defaultExpectedAction = {
id: SAVE_POST_NOTICE_ID,
id: 'editor-save',
actions: [],
type: 'snackbar',
};
Expand Down Expand Up @@ -106,7 +105,7 @@ describe( 'getNotificationArgumentsForSaveFail()', () => {
const error = { code: '42', message: 'Something went wrong.' };
const post = { status: 'publish' };
const edits = { status: 'publish' };
const defaultExpectedAction = { id: SAVE_POST_NOTICE_ID };
const defaultExpectedAction = { id: 'editor-save' };
[
[
'when error code is `rest_autosave_no_changes`',
Expand Down Expand Up @@ -190,7 +189,7 @@ describe( 'getNotificationArgumentsForTrashFail()', () => {
].forEach( ( [ description, error, message ] ) => {
// eslint-disable-next-line jest/valid-title
it( description, () => {
const expectedValue = [ message, { id: TRASH_POST_NOTICE_ID } ];
const expectedValue = [ message, { id: 'editor-trash-fail' } ];
expect( getNotificationArgumentsForTrashFail( { error } ) ).toEqual(
expectedValue
);
Expand Down
Loading