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

Gutenberg Data Tutorial 5: Adding a Delete Button #40940

Merged
merged 34 commits into from
Jul 1, 2022

Conversation

adamziel
Copy link
Contributor

@adamziel adamziel commented May 9, 2022

Description

This PR introduces the 5th part of Create your First App with Gutenberg Data tutorial – the focus is on deleting entity records.

Follows up on the fourth part: #39261 (sadly, the images aren't rendered properly there)

See the rich-text preview

@adamziel adamziel changed the title First draft of the fifth part of the Create Your First App With Gutenberg Data: "Adding a delete button" Gutenberg Data Tutorial 5: Adding a Delete Button May 9, 2022
@adamziel adamziel self-assigned this May 9, 2022
@adamziel adamziel added [Type] Developer Documentation Documentation for developers [Package] Core data /packages/core-data Developer Experience Ideas about improving block and theme developer experience labels May 9, 2022
@adamziel adamziel marked this pull request as ready for review May 17, 2022 13:13
@adamziel adamziel force-pushed the docs/adding-a-delete-button branch from a223e36 to 200fb4e Compare May 18, 2022 11:12
@adamziel
Copy link
Contributor Author

cc @getdave @scruffian @draganescu

@scruffian
Copy link
Contributor

This looks good. A couple of ideas:

  1. Hiding the item before the delete action has succeeded feels like a non-ideal pattern - what if the delete fails? What about changing the visual appearance of the line, rather than hiding it?

  2. It would be good to have a screencast of the whole flow at the end.

Copy link
Contributor

@getdave getdave left a comment

Choose a reason for hiding this comment

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

Great work as always. Easy to read and to follow. Left some nits.

{ isDeleting ? (
<>
<Spinner />
Deleting...
Copy link
Contributor

@draganescu draganescu Jun 30, 2022

Choose a reason for hiding this comment

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

Maybe for the sake of good practice we should use __( string ) for strings? Maybe it's too much for this tutorial tho.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@scruffian have already mentioned that – I'd like to spin another PR to update all the parts of this tutorial at once.

To tell the user when any of these happens, we need to extract the error information using the `getLastEntityDeleteError` selector:

```js
// Replace 9 with an actual page ID
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a bit confusing. Why not use a placeholder instead of this comment about the magic number?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How would you introduce a placeholder that would clearly communicate that?

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 left the placeholder for now, I'm happy to revisit later on, though!

Copy link
Contributor

@draganescu draganescu left a comment

Choose a reason for hiding this comment

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

Left a few nits but this is awesome otherwise!

@adamziel adamziel merged commit 4ca5ef6 into trunk Jul 1, 2022
@adamziel adamziel deleted the docs/adding-a-delete-button branch July 1, 2022 13:59
@github-actions github-actions bot added this to the Gutenberg 13.7 milestone Jul 1, 2022
@dmsnell
Copy link
Member

dmsnell commented Jul 6, 2022

Thanks for this great addition. One thing I noted is that the error handling code flashes an alert with useEffect( …, [error]) and I think that means we should expect an empty alert window when the error disappears. Might be best to try and omit any examples with alert() in them.

Alternatively we could use that to render a modal dialog, issue a toast when truthy, render an error message, or simply omit the rendering code for the example purposes.

Other than that I think these tutorials are getting so much more valuable 👍

@adamziel
Copy link
Contributor Author

adamziel commented Jul 6, 2022

@dmsnell Hm I was sure I removed the alert() calls from this PR in favor of the snackbar notifications. Unless you mean this part still talking about useEffect?

const DeletePageButton = ({ pageId }) => {
	// ...
	const { error, /* ... */ } = useSelect(
		select => ( {
			error: select( coreDataStore ).getLastEntityDeleteError( 'postType', 'page', pageId ),
			// ...
		} ),
		[pageId]
	);
	useEffect( () => {
		if ( error ) {
			// Display the error
		}
	}, [error] )
	// ...
}

If so – great spot, I'll rework it to be consistent with the error handling code later in the same document.

@dmsnell
Copy link
Member

dmsnell commented Jul 6, 2022

if you removed it I might have missed it because I followed the link in the description, which referenced a specific commit, so maybe it referenced an earlier version of the PR.

the main thing was in that useEffect block when the error disappears the code runs again so as long as we don't make it seem like it only runs when there is an error it's all good. you may have already done this.

@adamziel
Copy link
Contributor Author

adamziel commented Jul 6, 2022

It's already taken care of @dmsnell:

import { store as noticesStore } from '@wordpress/notices';
import { useEffect } from '@wordpress/element';
function DeletePageButton( { pageId } ) {
	const { createSuccessNotice, createErrorNotice } = useDispatch( noticesStore );
	// useSelect returns a list of selectors if you pass the store handle
	// instead of a callback:
	const { getLastEntityDeleteError } = useSelect( coreDataStore )
	const handleDelete = async () => {
		const success = await deleteEntityRecord( 'postType', 'page', pageId);
		if ( success ) {
			// Tell the user the operation succeeded:
			createSuccessNotice( "The page was deleted!", {
				type: 'snackbar',
			} );
		} else {
			// We use the selector directly to get the fresh error *after* the deleteEntityRecord
			// have failed.
			const lastError = getLastEntityDeleteError( 'postType', 'page', pageId );
			const message = ( lastError?.message || 'There was an error.' ) + ' Please refresh the page and try again.'
			// Tell the user how exactly the operation has failed:
			createErrorNotice( message, {
				type: 'snackbar',
			} );
		}
	}
	// ...
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Developer Experience Ideas about improving block and theme developer experience [Package] Core data /packages/core-data [Type] Developer Documentation Documentation for developers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants