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

Block Settings Menu: Adding a duplicate block button #5098

Merged
merged 5 commits into from Feb 26, 2018

Conversation

Projects
None yet
4 participants
@youknowriad
Contributor

youknowriad commented Feb 16, 2018

closes #2231

Testing instructions

  • Add any block
  • Open the block settings menu
  • Click "Duplicate"
  • A new block should be created right after the current block

It should also work for multiselection

@youknowriad youknowriad self-assigned this Feb 16, 2018

@youknowriad youknowriad requested a review from jasmussen Feb 16, 2018

@@ -486,6 +486,16 @@ export const getBlocks = createSelector(
]
);
export const getBlocksByUid = createSelector(

This comment has been minimized.

@aduth

aduth Feb 16, 2018

Member

More accurately: getBlocksByUids

Also need consensus on capitalization pattern getBlocksByUids vs. getBlocksByUIDs

This comment has been minimized.

@youknowriad

youknowriad Feb 16, 2018

Contributor

What's your preference on the capitalization, let's make it a guideline ;)?

This comment has been minimized.

@aduth

aduth Feb 16, 2018

Member

I agree a guideline would be good. I don't know which I prefer. I think capitalized abbreviations is more the norm, but there's exceptions. It also makes it difficult when a name has two adjacent abbreviations (imagine an XMLHTTPRequest).

🤷‍♂️

#2511

This comment has been minimized.

@aduth

aduth Feb 23, 2018

Member

I guess I'd slightly lean toward capitalized abbreviations.

index: getBlockIndex( state, last( ownProps.uids ), ownProps.rootUID ),
} ),
{ insertBlocks },
( { blocks, index }, dispatchProps, { rootUID } ) => ( {

This comment has been minimized.

@aduth

aduth Feb 16, 2018

Member

I see why it's used here, but I've becoming increasingly cautious of mergeProps upon discovery in #5090 about excessive renders it can incur.

This comment has been minimized.

@youknowriad

youknowriad Feb 16, 2018

Contributor

Another option would be to do this inside the component. Do you think it's better?

This comment has been minimized.

@aduth

aduth Feb 16, 2018

Member

Better in performance. Worse in having it become less purely presentational.

🤷‍♂️

This comment has been minimized.

@youknowriad

youknowriad Feb 21, 2018

Contributor

Is this blocking, I think we should keep it as is and revisit once we update the data module.

This comment has been minimized.

@aduth

aduth Feb 23, 2018

Member

Should be possible to simplify now with #5137

import { insertBlocks } from '../../store/actions';
export function BlockDuplicateButton( { onDuplicate, onClick = noop, isLocked, small = false } ) {
if ( isLocked ) {

This comment has been minimized.

@mtias

mtias Feb 21, 2018

Contributor

Should also check for useOnce here :)

This comment has been minimized.

@youknowriad

youknowriad Feb 22, 2018

Contributor

👍 Good catch

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Feb 22, 2018

This is VERY nice. I think this is going to be a very popular feature.

The block ellipsis menu is getting a little crowded, but that's due to the transformations rather than this addition. If we feel it's getting out of hand we can revisit having the transformations in this menu. Not for this PR.

One enhancement to consider for this, is selecting the new block after it's been duplicated. Especially if you multi select three blocks and duplicate, it'd be nice if the three duplicate blocks were simply still selected so you could move them around or whatever.

But no blocker.

👍 👍 nice work!

@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Feb 22, 2018

One enhancement to consider for this, is selecting the new block after it's been duplicated

It works for a single block, but I'll take a look to see if we can keep the multi-selection as well.

@jasmussen

This comment has been minimized.

Contributor

jasmussen commented Feb 22, 2018

It's not a requirement, just a nicety.

@mtias

This comment has been minimized.

Contributor

mtias commented Feb 22, 2018

I think we should do the flyout menu for transforms :)

@aduth

Most of my comments are minor. Seems to work well in my testing.

} ),
{ insertBlocks },
( { blocks, index }, dispatchProps, { rootUID } ) => ( {
blockNames: blocks.map( block => block.name ),

This comment has been minimized.

@aduth

aduth Feb 23, 2018

Member

Honestly, it would probably be more efficient to pass in the blocks array in its entirety from getBlocksByUid, since at least that's memoized, not returning a new instance each time.

const type = getBlockType( name );
return ! type.useOnce;
} ) );
if ( isLocked || ! canDuplicate ) {

This comment has been minimized.

@aduth

aduth Feb 23, 2018

Member

Minor (since it's not a common scenario): if isLocked is true here, we've wasted effort calculating canDuplicate, when we could have done an early return before the every.

@@ -486,6 +486,16 @@ export const getBlocks = createSelector(
]
);
export const getBlocksByUid = createSelector(

This comment has been minimized.

@aduth

aduth Feb 23, 2018

Member

I guess I'd slightly lean toward capitalized abbreviations.

return map( uids, ( uid ) => getBlock( state, uid ) );
},
( state ) => [
state.editor.present.blockOrder,

This comment has been minimized.

@aduth

aduth Feb 23, 2018

Member

How does this depend on blockOrder ?

This comment has been minimized.

@aduth

aduth Feb 23, 2018

Member

Also, since this depends on getBlock, it should define all of the dependants of getBlock.

Aside: rememo@3.0.0 adds a helper utility for this via getBlock.getDependants (changelog).

This comment has been minimized.

@youknowriad

youknowriad Feb 23, 2018

Contributor

I avoided using getDependants because of two things:

  • I have to map over the uids, to call the getDependants of the getBlock
  • I thought it's simpler to just add the global block state as dependants instead of using specific uids since a selector returning multiple blocks.
  • blockOrder is necessary because getBlock depends on blockOrder see getBlockDependantsCacheBust
@youknowriad

This comment has been minimized.

Contributor

youknowriad commented Feb 26, 2018

I feel like I addressed everything here. Merging.

@youknowriad youknowriad merged commit 6e273fc into master Feb 26, 2018

2 checks passed

codecov/project 39.46% (+4.98%) compared to ee3a61c
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@youknowriad youknowriad deleted the add/duplicate-button branch Feb 26, 2018

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