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.

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

Add save/error notifications to Navigation Menu page #21344

Closed
talldan opened this issue Apr 2, 2020 · 5 comments
Closed

Add save/error notifications to Navigation Menu page #21344

talldan opened this issue Apr 2, 2020 · 5 comments
Assignees
Labels
[Type] Enhancement A suggestion for improvement.

Comments

@talldan
Copy link
Contributor

talldan commented Apr 2, 2020

Description

Currently when saving a menu on the new Nav Menu page, there's no indication anything happened. Ideally:

  • The Save button would be disabled until changes that can be saved have been made
  • A Snackbar notice would appear when save is successful
  • A Notice notice would appear when a save error happens.

I had a look into this initially while working on #21342, I tried creating a react hook that used a store subscription, but it's not trivial to implement so I avoided adding more complexity to that PR.

Part of the difficulty come from the way MenuItems are saved one-by-one:

for ( const block of innerBlocks ) {
const menuItem = menuItemsRef.current[ block.clientId ];
if ( ! menuItem ) {
saveMenuItem( {
...createMenuItemAttributesFromBlock( block ),
menus: menuId,
} );
continue;
}
if (
! isEqual(
block.attributes,
createBlockFromMenuItem( menuItem ).attributes
)
) {
saveMenuItem( {
...menuItem,
...createMenuItemAttributesFromBlock( block ),
menus: menuId, // Gotta do this because REST API doesn't like receiving an array here. Maybe a bug in the REST API?
} );
}
}

A subscription wouldn't know which one of these indicates the final saving of the menu and when to show a snackbar notice.

It might be an idea to move this code to an action generator, like savePost in the block editor, and use the same approach for triggering notices:

/**
* Action generator for saving the current post in the editor.
*
* @param {Object} options
*/
export function* savePost( options = {} ) {
if ( ! ( yield select( STORE_KEY, 'isEditedPostSaveable' ) ) ) {
return;
}
let edits = {
content: yield select( STORE_KEY, 'getEditedPostContent' ),
};
if ( ! options.isAutosave ) {
yield dispatch( STORE_KEY, 'editPost', edits, { undoIgnore: true } );
}
yield __experimentalRequestPostUpdateStart( options );
const previousRecord = yield select( STORE_KEY, 'getCurrentPost' );
edits = {
id: previousRecord.id,
...( yield select(
'core',
'getEntityRecordNonTransientEdits',
'postType',
previousRecord.type,
previousRecord.id
) ),
...edits,
};
yield dispatch(
'core',
'saveEntityRecord',
'postType',
previousRecord.type,
edits,
options
);
yield __experimentalRequestPostUpdateFinish( options );
const error = yield select(
'core',
'getLastEntitySaveError',
'postType',
previousRecord.type,
previousRecord.id
);
if ( error ) {
const args = getNotificationArgumentsForSaveFail( {
post: previousRecord,
edits,
error,
} );
if ( args.length ) {
yield dispatch( 'core/notices', 'createErrorNotice', ...args );
}
} else {
const updatedRecord = yield select( STORE_KEY, 'getCurrentPost' );
const args = getNotificationArgumentsForSaveSuccess( {
previousPost: previousRecord,
post: updatedRecord,
postType: yield select( 'core', 'getPostType', updatedRecord.type ),
options,
} );
if ( args.length ) {
yield dispatch( 'core/notices', 'createSuccessNotice', ...args );
}
// Make sure that any edits after saving create an undo level and are
// considered for change detection.
if ( ! options.isAutosave ) {
yield dispatch(
'core/block-editor',
'__unstableMarkLastChangeAsPersistent'
);
}
}
}

@talldan talldan added [Type] Enhancement A suggestion for improvement. [Block] Navigation Affects the Navigation Block labels Apr 2, 2020
@adamziel
Copy link
Contributor

adamziel commented Apr 30, 2020

What about a partial success? As in saving a menu item initiates 10 requests and only 8 of them succeeds? Even with the most crafty notification this would still seem broken. I think we should consider taking a step back here, and starting with an atomic save operation which has a binary success / failure result. One idea would be

  • An API endpoint that accepts the entire navigation as a parameter
  • It starts a transaction and performs all the inserts / deletes / updates
  • If anything fails, it rolls back the entire transaction and returns a failure
  • If everything works, it commits the transaction and returns success

@talldan
Copy link
Contributor Author

talldan commented May 1, 2020

Very true @adamziel. The discussion here about the best way to save menus also ties into what's being discussed on #21964.

With partial success, we could consider showing errors on the menu items, but blocks don't really have a generic system for showing errors.

I agree a single request for saving would be nice.

@adamziel
Copy link
Contributor

adamziel commented May 4, 2020

Let's move the discussion there then :-)

@adamziel
Copy link
Contributor

adamziel commented May 4, 2020

Related PR from Andrei (Adds snackbar notification: #21557)

@adamziel
Copy link
Contributor

adamziel commented Jun 2, 2020

This is addressed by #22326 and #22603

@adamziel adamziel closed this as completed Jun 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Type] Enhancement A suggestion for improvement.
Projects
None yet
Development

No branches or pull requests

5 participants