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.

Sign up for GitHub

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

Consolidate disparate "copy block" actions #23088

Merged
merged 4 commits into from
Jun 12, 2020
Merged

Consolidate disparate "copy block" actions #23088

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
4f1cc30
make copied Block flash
ntsekouras Jun 11, 2020
60825bf
show copy Blocks notifications from block-actions Copy button
ntsekouras Jun 11, 2020
dcd525a
ClipboardButton make onFinishCopy optional attribute
ntsekouras Jun 12, 2020
b99fc90
Simplify onFinishCopy check
ntsekouras Jun 12, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
213 changes: 95 additions & 118 deletions packages/block-editor/src/components/block-actions/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,129 +6,106 @@ import { castArray, first, last, every } from 'lodash';
/**
* WordPress dependencies
*/
import { compose } from '@wordpress/compose';
import { withSelect, withDispatch } from '@wordpress/data';
import { useDispatch, useSelect } from '@wordpress/data';
import { hasBlockSupport, switchToBlockType } from '@wordpress/blocks';

function BlockActions( {
canDuplicate,
canInsertDefaultBlock,
children,
isLocked,
onDuplicate,
onGroup,
onInsertAfter,
onInsertBefore,
onRemove,
onUngroup,
blocks,
} ) {
/**
* Internal dependencies
*/
import { useNotifyCopy } from '../copy-handler';

export default function BlockActions( { clientIds, children } ) {
const {
canInsertBlockType,
getBlockRootClientId,
getBlocksByClientId,
getTemplateLock,
} = useSelect( ( select ) => select( 'core/block-editor' ), [] );
const {
getDefaultBlockName,
getGroupingBlockName,
} = useSelect( ( select ) => select( 'core/blocks' ) );

const blocks = getBlocksByClientId( clientIds );
const rootClientId = getBlockRootClientId( clientIds[ 0 ] );
const canDuplicate = every( blocks, ( block ) => {
return (
!! block &&
hasBlockSupport( block.name, 'multiple', true ) &&
canInsertBlockType( block.name, rootClientId )
);
} );

const canInsertDefaultBlock = canInsertBlockType(
getDefaultBlockName(),
rootClientId
);

const {
removeBlocks,
replaceBlocks,
duplicateBlocks,
insertAfterBlock,
insertBeforeBlock,
flashBlock,
} = useDispatch( 'core/block-editor' );

const notifyCopy = useNotifyCopy();

return children( {
canDuplicate,
canInsertDefaultBlock,
isLocked,
onDuplicate,
onGroup,
onInsertAfter,
onInsertBefore,
onRemove,
onUngroup,
isLocked: !! getTemplateLock( rootClientId ),
rootClientId,
blocks,
onDuplicate() {
return duplicateBlocks( clientIds );
},
onRemove() {
removeBlocks( clientIds );
},
onInsertBefore() {
insertBeforeBlock( first( castArray( clientIds ) ) );
},
onInsertAfter() {
insertAfterBlock( last( castArray( clientIds ) ) );
},
onGroup() {
if ( ! blocks.length ) {
return;
}

const groupingBlockName = getGroupingBlockName();

// Activate the `transform` on `core/group` which does the conversion
const newBlocks = switchToBlockType( blocks, groupingBlockName );

if ( ! newBlocks ) {
return;
}
replaceBlocks( clientIds, newBlocks );
},
onUngroup() {
if ( ! blocks.length ) {
return;
}

const innerBlocks = blocks[ 0 ].innerBlocks;

if ( ! innerBlocks.length ) {
return;
}

replaceBlocks( clientIds, innerBlocks );
},
onCopy() {
const selectedBlockClientIds = blocks.map(
( { clientId } ) => clientId
);
if ( blocks.length === 1 ) {
flashBlock( selectedBlockClientIds[ 0 ] );
}
notifyCopy( 'copy', selectedBlockClientIds );
Comment on lines +105 to +108
Copy link
Contributor

Choose a reason for hiding this comment

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

Have you weighed the benefits of combining notify and flash somewhere — maybe at the action level?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Of course I thought about combining them, but since flashBlock is used only if one block is copied/cut, didn't seem to me so consistent with the handling of the same actions with more blocks. So I think that CopyHandler might need more thought about this consistency in the future.

With the above said, I believe I need some more experience with the project, to understand it better and feel more comfortable before starting making more design decisions. I'm observing a bit more for now :)

Great thought provoking feedback Miguel! Really appreciate it!

},
} );
}

export default compose( [
withSelect( ( select, props ) => {
const {
canInsertBlockType,
getBlockRootClientId,
getBlocksByClientId,
getTemplateLock,
} = select( 'core/block-editor' );
const { getDefaultBlockName } = select( 'core/blocks' );

const blocks = getBlocksByClientId( props.clientIds );
const rootClientId = getBlockRootClientId( props.clientIds[ 0 ] );
const canDuplicate = every( blocks, ( block ) => {
return (
!! block &&
hasBlockSupport( block.name, 'multiple', true ) &&
canInsertBlockType( block.name, rootClientId )
);
} );

const canInsertDefaultBlock = canInsertBlockType(
getDefaultBlockName(),
rootClientId
);

return {
blocks,
canDuplicate,
canInsertDefaultBlock,
extraProps: props,
isLocked: !! getTemplateLock( rootClientId ),
rootClientId,
};
} ),
withDispatch( ( dispatch, props, { select } ) => {
const { clientIds, blocks } = props;

const {
removeBlocks,
replaceBlocks,
duplicateBlocks,
insertAfterBlock,
insertBeforeBlock,
} = dispatch( 'core/block-editor' );

return {
onDuplicate() {
return duplicateBlocks( clientIds );
},
onRemove() {
removeBlocks( clientIds );
},
onInsertBefore() {
insertBeforeBlock( first( castArray( clientIds ) ) );
},
onInsertAfter() {
insertAfterBlock( last( castArray( clientIds ) ) );
},
onGroup() {
if ( ! blocks.length ) {
return;
}

const { getGroupingBlockName } = select( 'core/blocks' );

const groupingBlockName = getGroupingBlockName();

// Activate the `transform` on `core/group` which does the conversion
const newBlocks = switchToBlockType(
blocks,
groupingBlockName
);

if ( ! newBlocks ) {
return;
}
replaceBlocks( clientIds, newBlocks );
},

onUngroup() {
if ( ! blocks.length ) {
return;
}

const innerBlocks = blocks[ 0 ].innerBlocks;

if ( ! innerBlocks.length ) {
return;
}

replaceBlocks( clientIds, innerBlocks );
},
};
} ),
] )( BlockActions );
13 changes: 3 additions & 10 deletions packages/block-editor/src/components/block-settings-menu/block-settings-dropdown.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ import {
} from '@wordpress/components';
import { useSelect } from '@wordpress/data';
import { moreHorizontal } from '@wordpress/icons';
import { useState } from '@wordpress/element';
import { serialize } from '@wordpress/blocks';

/**
Expand Down Expand Up @@ -57,8 +56,6 @@ export function BlockSettingsDropdown( { clientIds, ...props } ) {
};
}, [] );

const [ hasCopied, setHasCopied ] = useState();

return (
<BlockActions clientIds={ clientIds }>
{ ( {
Expand All @@ -69,6 +66,7 @@ export function BlockSettingsDropdown( { clientIds, ...props } ) {
onInsertAfter,
onInsertBefore,
onRemove,
onCopy,
blocks,
} ) => (
<DropdownMenu
Expand Down Expand Up @@ -99,14 +97,9 @@ export function BlockSettingsDropdown( { clientIds, ...props } ) {
text={ () => serialize( blocks ) }
role="menuitem"
className="components-menu-item__button"
onCopy={ () => {
setHasCopied( true );
} }
onFinishCopy={ () => setHasCopied( false ) }
onCopy={ onCopy }
>
{ hasCopied
? __( 'Copied!' )
: __( 'Copy' ) }
{ __( 'Copy' ) }
</ClipboardButton>
{ canDuplicate && (
<MenuItem
Expand Down
2 changes: 1 addition & 1 deletion packages/block-editor/src/components/copy-handler/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ import { __, sprintf } from '@wordpress/i18n';
*/
import { getPasteEventData } from '../../utils/get-paste-event-data';

function useNotifyCopy() {
export function useNotifyCopy() {
Copy link
Contributor

Choose a reason for hiding this comment

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

With useNotifyCopy being used in more than one component, is copy-handler still the right place for its definition? (Also, is this hook a good abstraction to begin with? 🙂)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hey Miguel, thanks for the feedback!

useNotifyCopy could certainly go to a different file, under copy-handler folder ( even if it wasn't reused ) and maybe renamed to reflect that handles cut notifications too.

I believe it's a good abstraction as it does one thing: notifies the user of a copy/cut action. Every such action should have consistency about the way a user is notified.

const { getBlockName } = useSelect(
( select ) => select( 'core/block-editor' ),
[]
Expand Down
2 changes: 1 addition & 1 deletion packages/components/src/clipboard-button/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ export default function ClipboardButton( {

if ( hasCopied ) {
onCopy();
} else {
} else if ( onFinishCopy ) {
onFinishCopy();
}

Expand Down