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 Reusable Block deletion #4139

Merged
merged 9 commits into from Jan 4, 2018

Conversation

Projects
None yet
3 participants
@noisysocks
Member

noisysocks commented Dec 22, 2017

✨ 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:

  1. There's a new DELETE_REUSABLE_BLOCK effect which calls the destroy() method that the WP API Backbone library gives us for free.
  2. 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:

screen shot 2017-12-21 at 12 29 19

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

screen shot 2017-12-21 at 12 29 46

πŸ“‹ How to test

  1. Create a block
  2. Convert it to a reusable block
  3. Rename the reusable block
  4. Edit the reusable block
  5. Use the Inserter to insert a copy of the reusable block
  6. Remove a block
  7. Delete the reusable block
@noisysocks

This comment has been minimized.

Show comment
Hide comment
@noisysocks

noisysocks Dec 22, 2017

Member

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

Member

noisysocks commented Dec 22, 2017

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

@noisysocks noisysocks changed the base branch from add/reusable-block-deletion-to-api to master Dec 27, 2017

@aduth

aduth requested changes Jan 2, 2018 edited

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:

image

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.
Show outdated Hide outdated editor/components/block-settings-menu/reusable-block-settings.js Outdated
Show outdated Hide outdated editor/store/reducer.js Outdated
Show outdated Hide outdated editor/store/test/effects.js Outdated
Show outdated Hide outdated editor/store/effects.js Outdated

noisysocks added some commits Dec 20, 2017

Add DELETE_REUSABLE_BLOCK effect
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.
Add UI for deleting a reusable block
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'
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
Makes the REMOVE_REUSABLE_BLOCK action also remove any blocks that are
using the removed Reusable Block from the editor state.
Prevent temporary reusable blocks from being deleted
Don't allow DELETE_REUSABLE_BLOCK to delete a reusable block that is
marked temporary.
Simplify `reusableBlocks.isFetching` reducer
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
We can avoid mocking `uniqueId` by being less pedantic about what our
tests expect.
@noisysocks

This comment has been minimized.

Show comment
Hide comment
@noisysocks

noisysocks Jan 4, 2018

Member

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

Member

noisysocks commented Jan 4, 2018

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

aduth approved these changes Jan 4, 2018

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

This comment has been minimized.

Show comment
Hide comment
@noisysocks

noisysocks Jan 4, 2018

Member

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.

Member

noisysocks commented Jan 4, 2018

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 noisysocks merged commit a6948bd into master Jan 4, 2018

3 checks passed

codecov/project 39.41% (+0.22%) compared to 7557026
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details

@noisysocks noisysocks deleted the add/reusable-block-deletion branch Jan 4, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment