Add Reusable Block deletion #4139

noisysocks · 2017-12-22T05:47:12Z

✨ What this is

Special 2-for-1 offer: closes #3792 and closes #4041.

~~Note that I've set add/reusable-block-deletion-to-api as the base branch so that the diff is bearable. The intention is to merge this into master after #4137 lands.~~

🗑 Adds the ability to delete a Reusable Block

This PR uses the new API introduced in #4137 to add support for deleting Reusable Blocks.

With the new API, this is pretty straightforward:

There's a new DELETE_REUSABLE_BLOCK effect which calls the destroy() method that the WP API Backbone library gives us for free.
This effect optimistically calls REMOVE_REUSABLE_BLOCK which removes the Reusable Block and any blocks that reference it from local editor state. If the delete fails, REMOVE_REUSABLE_BLOCK is reverted.

💆‍♀️ Improves the Reusable Block flow

I implemented the changes suggested by @karmatosed in #4041 by moving 'Detach Reusable Block' into a seperate settings menu section alongside our new 'Delete Reusable Block' button.

Here's how the settings menu now looks for a static block:

And here's how it now looks for a reusable block:

📋 How to test

Create a block
Convert it to a reusable block
Rename the reusable block
Edit the reusable block
Use the Inserter to insert a copy of the reusable block
Remove a block
Delete the reusable block

noisysocks · 2017-12-22T05:48:38Z

cc. @youknowriad @aduth @pento since they know this code
cc. @karmatosed @jasmussen for UX feedback

aduth

An edge case, but one that could be potentially destructive: We allow the user to delete a reusable block while it's still in its "temporary" unsaved state, meaning that it will use the value produced by uniqueId. Since a block could exist with an ID matching the temporary ID, the user could inadvertently delete the wrong reusable block.

Luckily I didn't have a block with ID 2:

I'd suggest:

Prevent deleting reusable block if it hasn't been saved yet.
Maybe pass the argument to uniqueId, which acts as a prefix. Arguably this shouldn't be necessary, but eliminates any chance that the temproary ID overlaps with a real block ID.

aduth · 2018-01-02T21:16:57Z

editor/components/block-settings-menu/reusable-block-settings.js

+		onDelete( id ) {
+			// TODO: Make this a <Confirm /> component or similar
+			// eslint-disable-next-line no-alert
+			if ( window.confirm( __( 'Are you sure you want to permanently delete this Reusable Block?' ) ) ) {


You might be interested in a related discussion on "Scariness" of such prompts:

Automattic/wp-calypso#10982

In other words, should we be proactive about messaging here to clarify that deleting a block can impact other posts?

Good idea! Made the copy scarier in 6c0e185.

aduth · 2018-01-02T21:27:15Z

editor/store/reducer.js

+			case 'FETCH_REUSABLE_BLOCKS_FAILURE': {
+				const { id } = action;
+				if ( ! id ) {
+					return state;


In what situations would you expect this code path to be hit? Even if it was falsey, would there be harm in allowing omit to proceed? Aside from returning a new reference: But there's also an issue that if id is truthy but doesn't exist in the current set, we still return a new reference.

omit( state, 'some-id-that-doesnt-exist' ) !== state

You're right, this check isn't necessary. I've removed it in 92b6a18.

aduth · 2018-01-02T21:30:20Z

editor/store/test/effects.js

@@ -471,6 +474,8 @@ describe( 'effects', () => {

 	describe( 'reusable block effects', () => {
 		beforeAll( () => {
+			lodash.uniqueId = jest.spyOn( lodash, 'uniqueId' );


I wonder if we could achieve this more concisely with a combination of jest.mock and require.requireActual.

But then, looking at how you're asserting on optimist.id, do we really need this at all? Since you're using expect.any( Object ), and not asserting against the actual value produced by uniqueId.

You're right, mocking this isn't giving us much benefit considering the effort involved. I've removed the mock by making our assertions less strict in 78a9bb7.

chriskmnds · 2018-01-03T18:04:55Z

editor/store/effects.js

+
+		const allBlocks = getBlocks( getState() );
+		const associatedBlocks = allBlocks.filter( block => isReusableBlock( block ) && block.attributes.ref === id );
+		const associatedBlockUids = associatedBlocks.map( block => block.uid );


Might a reducer here (i.e. allBlocks.reduce...) help avoid passing through the blocks list twice? Or is that a heavier operation? Just a tiny thought on optimisation I guess :-)

You're absolutely right that using reduce would likely be faster!

Still, I prefer to optimise for readability over performance unless it's an operation that's performance sensitive. Deleting a reusable block is a fairly uncommon operation, so I'm not too worried.

Adds an effect (DELETE_REUSABLE_BLOCK) which triggers deletes a reusable block via the API. The action that triggers this effect can be created with the deleteReusableBlock() action creator. The reusable block is optimistically removed from local redux state by the reducer handling the REMOVE_REUSABLE_BLOCK action.

Moves 'Detach from Reusable Block' and adds 'Delete Reusable Block' buttons to a <ReusableBlockSettings> component that lives inside <BlockSettingsMenu>.

This helps reduce the ambiguity between removing a block and deleting a Reusable Block.

Makes the REMOVE_REUSABLE_BLOCK action also remove any blocks that are using the removed Reusable Block from the editor state.

Don't allow DELETE_REUSABLE_BLOCK to delete a reusable block that is marked temporary.

There's no need to check for the existence of `id` before `omit`ing it. If `id` is falsey then `omit` will do nothing.

We can avoid mocking `uniqueId` by being less pedantic about what our tests expect.

noisysocks · 2018-01-04T03:02:20Z

Thanks for reviewing, @aduth! I've addressed your comments and made it so that reusable blocks with temporary IDs cannot be deleted in 1f9e5c2.

aduth

Nice work 👍

Question: Should the deleted block be removed from state.preferences.blockUsage as well? I've noticed there's some issues with Reusable Blocks in recent and frequently used blocks, so wasn't sure if you planned to address this separately. I'd guess that the reusable block of state.preferences.blockUsage is not storing the ref anyways, so not tracking details of the block being removed.

noisysocks · 2018-01-04T22:30:55Z

Question: Should the deleted block be removed from state.preferences.blockUsage as well?

That's a good point. I'll do this as part of #4224 since, as you say, we aren't currently tracking reusable blocks in the frequent/recent list of blocks.

noisysocks added [Feature] Synced Patterns Related to synced patterns (formerly reusable blocks) [Type] Enhancement A suggestion for improvement. labels Dec 22, 2017

noisysocks changed the base branch from add/reusable-block-deletion-to-api to master December 27, 2017 23:19

aduth requested changes Jan 2, 2018

View reviewed changes

chriskmnds reviewed Jan 3, 2018

View reviewed changes

noisysocks added 9 commits January 4, 2018 12:16

Add UI for deleting a reusable block

528d65f

Moves 'Detach from Reusable Block' and adds 'Delete Reusable Block' buttons to a <ReusableBlockSettings> component that lives inside <BlockSettingsMenu>.

Rename 'Delete' button in block settings menu to 'Remove'

a0e20fd

This helps reduce the ambiguity between removing a block and deleting a Reusable Block.

When deleting a Reusable Block, remove associated blocks from the post

83a55fd

Makes the REMOVE_REUSABLE_BLOCK action also remove any blocks that are using the removed Reusable Block from the editor state.

Make success notice say 'Block created' when the Reusable Block is new

48d2102

Prevent temporary reusable blocks from being deleted

1f9e5c2

Don't allow DELETE_REUSABLE_BLOCK to delete a reusable block that is marked temporary.

Make Reusable Block delete confirmation scarier

6c0e185

Simplify reusableBlocks.isFetching reducer

92b6a18

There's no need to check for the existence of `id` before `omit`ing it. If `id` is falsey then `omit` will do nothing.

Don't mock lodash.uniqueId

78a9bb7

We can avoid mocking `uniqueId` by being less pedantic about what our tests expect.

noisysocks force-pushed the add/reusable-block-deletion branch from e81ad82 to 78a9bb7 Compare January 4, 2018 02:59

aduth approved these changes Jan 4, 2018

View reviewed changes

aduth mentioned this pull request Jan 4, 2018

Element Package: Add the Gutenberg Element Module as a reusable package WordPress/packages#57

Closed

noisysocks merged commit a6948bd into master Jan 4, 2018

noisysocks deleted the add/reusable-block-deletion branch January 4, 2018 22:31

noisysocks mentioned this pull request Jan 4, 2018

Refactor InserterMenu & VisualEditorInserter; allow recent/frequent reusable blocks to be inserted #4224

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add Reusable Block deletion #4139

Add Reusable Block deletion #4139

noisysocks commented Dec 22, 2017 •

edited

Loading

noisysocks commented Dec 22, 2017

aduth left a comment •

edited

Loading

aduth Jan 2, 2018

noisysocks Jan 4, 2018

aduth Jan 2, 2018

noisysocks Jan 4, 2018

aduth Jan 2, 2018

noisysocks Jan 4, 2018 •

edited

Loading

chriskmnds Jan 3, 2018

noisysocks Jan 4, 2018 •

edited

Loading

noisysocks commented Jan 4, 2018

aduth left a comment

noisysocks commented Jan 4, 2018

Add Reusable Block deletion #4139

Add Reusable Block deletion #4139

Conversation

noisysocks commented Dec 22, 2017 • edited Loading

✨ What this is

🗑 Adds the ability to delete a Reusable Block

💆‍♀️ Improves the Reusable Block flow

📋 How to test

noisysocks commented Dec 22, 2017

aduth left a comment • edited Loading

Choose a reason for hiding this comment

aduth Jan 2, 2018

Choose a reason for hiding this comment

noisysocks Jan 4, 2018

Choose a reason for hiding this comment

aduth Jan 2, 2018

Choose a reason for hiding this comment

noisysocks Jan 4, 2018

Choose a reason for hiding this comment

aduth Jan 2, 2018

Choose a reason for hiding this comment

noisysocks Jan 4, 2018 • edited Loading

Choose a reason for hiding this comment

chriskmnds Jan 3, 2018

Choose a reason for hiding this comment

noisysocks Jan 4, 2018 • edited Loading

Choose a reason for hiding this comment

noisysocks commented Jan 4, 2018

aduth left a comment

Choose a reason for hiding this comment

noisysocks commented Jan 4, 2018

noisysocks commented Dec 22, 2017 •

edited

Loading

aduth left a comment •

edited

Loading

noisysocks Jan 4, 2018 •

edited

Loading

noisysocks Jan 4, 2018 •

edited

Loading