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

Jump to bottom

Try: Reusable block edit locking #39950

Merged
merged 7 commits into from
Apr 11, 2022
Merged

Try: Reusable block edit locking #39950

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
1eb0057
Introduce canEditBlock selector
Mamaduka Mar 31, 2022
eb392d3
Update BlockContentOverlay
Mamaduka Mar 31, 2022
19610a9
Update UI
Mamaduka Mar 31, 2022
4162705
Fix indeterminate state and e2e tests
Mamaduka Apr 3, 2022
3b84542
Update list view
Mamaduka Apr 11, 2022
4656245
Update selector
Mamaduka Apr 11, 2022
71ebda8
Provide 'isLocked' value via hook
Mamaduka Apr 11, 2022
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
13 changes: 13 additions & 0 deletions docs/reference-guides/data/data-core-block-editor.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,19 @@ _Returns_

- `boolean`: True if the block has controlled inner blocks.

### canEditBlock

Determines if the given block is allowed to be edited.

_Parameters_

- _state_ `Object`: Editor state.
- _clientId_ `string`: The block client Id.

_Returns_

- `boolean`: Whether the given block is allowed to be edited.

### canInsertBlocks

Determines if the given blocks are allowed to be inserted into the block
Expand Down
21 changes: 19 additions & 2 deletions packages/block-editor/src/components/block-content-overlay/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ export default function BlockContentOverlay( {
const [ isHovered, setIsHovered ] = useState( false );

const {
canEdit,
isParentSelected,
hasChildSelected,
isDraggingBlocks,
Expand All @@ -36,8 +37,10 @@ export default function BlockContentOverlay( {
hasSelectedInnerBlock,
isDraggingBlocks: _isDraggingBlocks,
isBlockHighlighted,
canEditBlock,
} = select( blockEditorStore );
return {
canEdit: canEditBlock( clientId ),
isParentSelected: isBlockSelected( clientId ),
hasChildSelected: hasSelectedInnerBlock( clientId, true ),
isDraggingBlocks: _isDraggingBlocks(),
Expand All @@ -59,6 +62,12 @@ export default function BlockContentOverlay( {
);

useEffect( () => {
// The overlay is always active when editing is locked.
if ( ! canEdit ) {
setIsOverlayActive( true );
return;
}

// Reenable when blocks are not in use.
if ( ! isParentSelected && ! hasChildSelected && ! isOverlayActive ) {
setIsOverlayActive( true );
Expand All @@ -75,7 +84,13 @@ export default function BlockContentOverlay( {
if ( hasChildSelected && isOverlayActive ) {
setIsOverlayActive( false );
}
}, [ isParentSelected, hasChildSelected, isOverlayActive, isHovered ] );
}, [
isParentSelected,
hasChildSelected,
isOverlayActive,
isHovered,
canEdit,
] );

// Disabled because the overlay div doesn't actually have a role or functionality
// as far as the a11y is concerned. We're just catching the first click so that
Expand All @@ -88,7 +103,9 @@ export default function BlockContentOverlay( {
onMouseEnter={ () => setIsHovered( true ) }
onMouseLeave={ () => setIsHovered( false ) }
onMouseUp={
isOverlayActive ? () => setIsOverlayActive( false ) : undefined
isOverlayActive && canEdit
Copy link
Contributor

Choose a reason for hiding this comment

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

Is canEdit needed here? The useEffect above seems to set isOverlayActive back to true even if it becomes false here. Or is this check meant to prevent this tiny blip of isOverLayActive being set to false and then right away to true?

Copy link
Member Author

Choose a reason for hiding this comment

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

Or is this check meant to prevent this tiny blip of isOverLayActive being set to false and then right away to true

Correct. It's not always noticeable, but there's also no need to attach an event callback when editing is disabled.

? () => setIsOverlayActive( false )
: undefined
}
>
{ wrapperProps?.children }
Expand Down
7 changes: 5 additions & 2 deletions packages/block-editor/src/components/block-lock/menu-item.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,11 @@ import useBlockLock from './use-block-lock';
import BlockLockModal from './modal';

export default function BlockLockMenuItem( { clientId } ) {
const { canMove, canRemove, canLock } = useBlockLock( clientId, true );
const isLocked = ! canMove || ! canRemove;
const { canEdit, canMove, canRemove, canLock } = useBlockLock(
clientId,
true
);
const isLocked = ! canEdit || ! canMove || ! canRemove;
Copy link
Contributor

@tomasztunik tomasztunik Apr 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Would it make sense to return isLocked from the useBlockLock hook? It would offload responsibility for managing this to a single source. If we introduce more options it'll require changing in a single place.

Copy link
Contributor

Choose a reason for hiding this comment

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

we also use it as isAllChecked in another place

Copy link
Contributor

@tomasztunik tomasztunik Apr 11, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

and as inversion of isLocked canEdit && canMove && canRemove
(ok this is not entirely true)


const [ isModalOpen, toggleModal ] = useReducer(
( isActive ) => ! isActive,
Expand Down
50 changes: 44 additions & 6 deletions packages/block-editor/src/components/block-lock/modal.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,8 @@ import {
} from '@wordpress/components';
import { lock as lockIcon, unlock as unlockIcon } from '@wordpress/icons';
import { useInstanceId } from '@wordpress/compose';
import { useDispatch } from '@wordpress/data';
import { useDispatch, useSelect } from '@wordpress/data';
import { isReusableBlock, getBlockType } from '@wordpress/blocks';

/**
* Internal dependencies
Expand All @@ -24,7 +25,18 @@ import { store as blockEditorStore } from '../../store';

export default function BlockLockModal( { clientId, onClose } ) {
const [ lock, setLock ] = useState( { move: false, remove: false } );
const { canMove, canRemove } = useBlockLock( clientId, true );
const { canEdit, canMove, canRemove } = useBlockLock( clientId, true );
const { isReusable } = useSelect(
( select ) => {
const { getBlockName } = select( blockEditorStore );
const blockName = getBlockName( clientId );

return {
isReusable: isReusableBlock( getBlockType( blockName ) ),
};
},
[ clientId ]
);
const { updateBlockAttributes } = useDispatch( blockEditorStore );
const blockInformation = useBlockDisplayInformation( clientId );
const instanceId = useInstanceId(
Expand All @@ -36,12 +48,12 @@ export default function BlockLockModal( { clientId, onClose } ) {
setLock( {
move: ! canMove,
remove: ! canRemove,
...( isReusable ? { edit: ! canEdit } : {} ),
} );
}, [ canMove, canRemove ] );
}, [ canEdit, canMove, canRemove, isReusable ] );

const isAllChecked = Object.values( lock ).every( Boolean );
Copy link
Contributor

Choose a reason for hiding this comment

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

could be replaced with isLocked

Copy link
Member Author

Choose a reason for hiding this comment

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

The effect uses these values, not derived one.

const isIndeterminate =
Object.values( lock ).some( Boolean ) && ! isAllChecked;
const isMixed = Object.values( lock ).some( Boolean ) && ! isAllChecked;

return (
<Modal
Expand Down Expand Up @@ -77,15 +89,41 @@ export default function BlockLockModal( { clientId, onClose } ) {
<span id={ instanceId }>{ __( 'Lock all' ) }</span>
}
checked={ isAllChecked }
indeterminate={ isIndeterminate }
indeterminate={ isMixed }
onChange={ ( newValue ) =>
setLock( {
move: newValue,
remove: newValue,
...( isReusable ? { edit: newValue } : {} ),
} )
}
/>
<ul className="block-editor-block-lock-modal__checklist">
{ isReusable && (
<li className="block-editor-block-lock-modal__checklist-item">
<CheckboxControl
label={
<>
{ __( 'Restrict editing' ) }
<Icon
icon={
lock.edit
? lockIcon
: unlockIcon
}
/>
</>
}
checked={ !! lock.edit }
onChange={ ( edit ) =>
setLock( ( prevLock ) => ( {
...prevLock,
edit,
} ) )
}
/>
</li>
) }
<li className="block-editor-block-lock-modal__checklist-item">
<CheckboxControl
label={
Expand Down
4 changes: 2 additions & 2 deletions packages/block-editor/src/components/block-lock/toolbar.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ import useBlockDisplayInformation from '../use-block-display-information';

export default function BlockLockToolbar( { clientId } ) {
const blockInformation = useBlockDisplayInformation( clientId );
const { canMove, canRemove, canLock } = useBlockLock( clientId );
const { canEdit, canMove, canRemove, canLock } = useBlockLock( clientId );

const [ isModalOpen, toggleModal ] = useReducer(
( isActive ) => ! isActive,
Expand All @@ -26,7 +26,7 @@ export default function BlockLockToolbar( { clientId } ) {
return null;
}

if ( canMove && canRemove ) {
if ( canEdit && canMove && canRemove ) {
return null;
}

Expand Down
2 changes: 2 additions & 0 deletions packages/block-editor/src/components/block-lock/use-block-lock.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ export default function useBlockLock( clientId, checkParent = false ) {
return useSelect(
( select ) => {
const {
canEditBlock,
canMoveBlock,
canRemoveBlock,
canLockBlockType,
Expand All @@ -32,6 +33,7 @@ export default function useBlockLock( clientId, checkParent = false ) {
: null;

return {
canEdit: canEditBlock( clientId ),
canMove: canMoveBlock( clientId, rootClientId ),
canRemove: canRemoveBlock( clientId, rootClientId ),
canLock: canLockBlockType( getBlockName( clientId ) ),
Expand Down
4 changes: 2 additions & 2 deletions packages/block-editor/src/components/list-view/block-select-button.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,8 @@ function ListViewBlockSelectButton(
ref
) {
const blockInformation = useBlockDisplayInformation( clientId );
const { canMove, canRemove } = useBlockLock( clientId );
const isLocked = ! canMove || ! canRemove;
const { canEdit, canMove, canRemove } = useBlockLock( clientId );
const isLocked = ! canEdit || ! canMove || ! canRemove;
Mamaduka marked this conversation as resolved.
Show resolved Hide resolved

// The `href` attribute triggers the browser's native HTML drag operations.
// When the link is dragged, the element's outerHTML is set in DataTransfer object as text/html.
Expand Down
24 changes: 24 additions & 0 deletions packages/block-editor/src/store/selectors.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1569,6 +1569,30 @@ export function canMoveBlocks( state, clientIds, rootClientId = null ) {
);
}

/**
* Determines if the given block is allowed to be edited.
*
* @param {Object} state Editor state.
* @param {string} clientId The block client Id.
*
* @return {boolean} Whether the given block is allowed to be edited.
*/
export function canEditBlock( state, clientId ) {
const attributes = getBlockAttributes( state, clientId );
if ( attributes === null ) {
return true;
}

const { lock } = attributes;
// No `lock` attribute means we can edit the block.
if ( lock === undefined || lock?.edit === undefined ) {
adamziel marked this conversation as resolved.
Show resolved Hide resolved
return true;
}

// When the edit is true, we cannot edit the block.
return ! lock?.edit;
}

/**
* Determines if the given block type can be locked/unlocked by a user.
*
Expand Down