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

Fix inconsistent Link UI in Nav block list view editor #50774

Merged
merged 15 commits into from
May 19, 2023
Merged

Fix inconsistent Link UI in Nav block list view editor #50774

Show file tree
Hide file tree
Changes from 12 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
2ae86a7
Move inserted block getter/setter to context
getdave May 19, 2023
fd385b8
Pass insertedBlock state to render prop callback
getdave May 19, 2023
7d10355
Conditionalise Link UI based on insertedBlock state from List View
getdave May 19, 2023
b3fa26c
Remove effect-ful code and rely on props passed to render callback
getdave May 19, 2023
040ef15
Completely remove depedency on global editor state
getdave May 19, 2023
3289877
Trigger Link UI on submenu insertion
getdave May 19, 2023
9f6b8e0
Remove console logging
getdave May 19, 2023
836504b
Add context via comments to potentially obsecure drilled prop
getdave May 19, 2023
3796974
Remove unused hook
getdave May 19, 2023
a8d3c7b
Augment e2e tests to provide coverage for both bugs
getdave May 19, 2023
6afee4d
Improve context around bug coverage in test
getdave May 19, 2023
0ad0df7
Align comment ordering with code ordering
getdave May 19, 2023
8e116ee
Remove redundant private Selector from Store
getdave May 19, 2023
8f45485
Fix comment typos
getdave May 19, 2023
8c6122c
Remove check to see if the block has a url, as it should never have a…
jeryj May 19, 2023
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
5 changes: 3 additions & 2 deletions packages/block-editor/src/components/list-view/appender.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,19 +4,20 @@
import { useInstanceId } from '@wordpress/compose';
import { speak } from '@wordpress/a11y';
import { useSelect } from '@wordpress/data';
import { forwardRef, useState, useEffect } from '@wordpress/element';
import { forwardRef, useEffect } from '@wordpress/element';
import { __, sprintf } from '@wordpress/i18n';

/**
* Internal dependencies
*/
import { store as blockEditorStore } from '../../store';
import useBlockDisplayTitle from '../block-title/use-block-display-title';
import { useListViewContext } from './context';
import Inserter from '../inserter';

export const Appender = forwardRef(
( { nestingLevel, blockCount, clientId, ...props }, ref ) => {
const [ insertedBlock, setInsertedBlock ] = useState( null );
const { insertedBlock, setInsertedBlock } = useListViewContext();

const instanceId = useInstanceId( Appender );
const { hideInserter } = useSelect(
Expand Down
10 changes: 8 additions & 2 deletions packages/block-editor/src/components/list-view/block-contents.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,8 @@ const ListViewBlockContents = forwardRef(
[ clientId ]
);

const { renderAdditionalBlockUI } = useListViewContext();
const { renderAdditionalBlockUI, insertedBlock, setInsertedBlock } =
useListViewContext();

const isBlockMoveTarget =
blockMovingClientId && selectedBlockInBlockEditor === clientId;
Expand All @@ -66,7 +67,12 @@ const ListViewBlockContents = forwardRef(

return (
<>
{ renderAdditionalBlockUI && renderAdditionalBlockUI( block ) }
{ renderAdditionalBlockUI &&
renderAdditionalBlockUI(
block,
insertedBlock,
setInsertedBlock
Comment on lines +71 to +74
Copy link
Contributor

Choose a reason for hiding this comment

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

This now forms part of the API of renderAdditionalBlockUI as positional params. If and when renderAdditionalBlockUI becomes part of the stable API, insertedBlock and setInsertedBlock might not be applicable for some use cases, but we also might want to add additional params at some stage. Would it be worth switching renderAdditionalBlockUI to use an object as its param?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the feedback on this. When I implemented this I also considered that but (for a reasons I can't recall) neglected to include it in the PR 😕

I think it's a good idea. The useful state that might need to be made available to this render prop in future might be many things so we should convert to object.

As for using a component - that seems logical to me. This PR didn't implement that but it's definitely worth calling out. It's very similar to the renderAppender arg of InnerBlocks so we could follow the pattern set down there.

Copy link
Contributor

Choose a reason for hiding this comment

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

Nice. Thanks for opening up the follow-up issue!

) }
<BlockDraggable clientIds={ draggableClientIds }>
{ ( { draggable, onDragStart, onDragEnd } ) => (
<ListViewBlockSelectButton
Expand Down
2 changes: 2 additions & 0 deletions packages/block-editor/src/components/list-view/block.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,6 +135,7 @@ function ListViewBlock( {
BlockSettingsMenu,
listViewInstanceId,
expandedState,
setInsertedBlock,
} = useListViewContext();

const hasSiblings = siblingBlockCount > 0;
Expand Down Expand Up @@ -339,6 +340,7 @@ function ListViewBlock( {
__experimentalSelectBlock={ updateSelection }
expand={ expand }
expandedState={ expandedState }
setInsertedBlock={ setInsertedBlock }
/>
) }
</TreeGridCell>
Expand Down
8 changes: 8 additions & 0 deletions packages/block-editor/src/components/list-view/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ import {
useRef,
useReducer,
forwardRef,
useState,
} from '@wordpress/element';
import { __ } from '@wordpress/i18n';

Expand Down Expand Up @@ -128,6 +129,9 @@ function ListViewComponent(
const treeGridRef = useMergeRefs( [ elementRef, dropZoneRef, ref ] );

const isMounted = useRef( false );

const [ insertedBlock, setInsertedBlock ] = useState( null );

const { setSelectedTreeId } = useListViewExpandSelectedItem( {
firstSelectedBlockClientId: selectedClientIds[ 0 ],
setExpandedState,
Expand Down Expand Up @@ -212,6 +216,8 @@ function ListViewComponent(
BlockSettingsMenu,
listViewInstanceId: instanceId,
renderAdditionalBlockUI,
insertedBlock,
setInsertedBlock,
} ),
[
draggedClientIds,
Expand All @@ -221,6 +227,8 @@ function ListViewComponent(
BlockSettingsMenu,
instanceId,
renderAdditionalBlockUI,
insertedBlock,
setInsertedBlock,
Copy link
Contributor

Choose a reason for hiding this comment

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

We are stuffing even more stuff into context. Is that a problem?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's always a potential problem for performance because if the context changes and components which consume it (or their children) will be re-rendered.

The setter is stable, but the insertedBlock will change.

However, pre-optimising context is challenging. Better to include in the memo and then optimise if/when List View becomes a problem.

]
);

Expand Down
43 changes: 0 additions & 43 deletions packages/block-library/src/navigation-link/use-inserted-block.js
View file
Open in desktop

This file was deleted.

15 changes: 14 additions & 1 deletion packages/block-library/src/navigation/edit/leaf-more-menu.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,13 @@ const BLOCKS_THAT_CAN_BE_CONVERTED_TO_SUBMENU = [
'core/navigation-submenu',
];

function AddSubmenuItem( { block, onClose, expandedState, expand } ) {
function AddSubmenuItem( {
getdave marked this conversation as resolved.
Show resolved Hide resolved
block,
onClose,
expandedState,
expand,
setInsertedBlock,
} ) {
const { insertBlock, replaceBlock, replaceInnerBlocks } =
useDispatch( blockEditorStore );

Expand Down Expand Up @@ -69,6 +75,12 @@ function AddSubmenuItem( { block, onClose, expandedState, expand } ) {
updateSelectionOnInsert
);
}

// This call sets the local List View state for the "last inserted block".
// This is required for the Nav Block to determine whether or not to display
// the Link UI for this new block.
setInsertedBlock( newLink );

if ( ! expandedState[ block.clientId ] ) {
expand( block.clientId );
}
Expand Down Expand Up @@ -138,6 +150,7 @@ export default function LeafMoreMenu( props ) {
expanded
expandedState={ props.expandedState }
expand={ props.expand }
setInsertedBlock={ props.setInsertedBlock }
/>
</MenuGroup>
<MenuGroup>
Expand Down
83 changes: 38 additions & 45 deletions packages/block-library/src/navigation/edit/menu-inspector-controls.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,7 @@ import {
__experimentalHeading as Heading,
Spinner,
} from '@wordpress/components';
import { useSelect } from '@wordpress/data';
import { useState, useEffect } from '@wordpress/element';
import { useSelect, useDispatch } from '@wordpress/data';
import { __, sprintf } from '@wordpress/i18n';

/**
Expand All @@ -26,7 +25,6 @@ import useNavigationMenu from '../use-navigation-menu';
import LeafMoreMenu from './leaf-more-menu';
import { updateAttributes } from '../../navigation-link/update-attributes';
import { LinkUI } from '../../navigation-link/link-ui';
import { useInsertedBlock } from '../../navigation-link/use-inserted-block';

/* translators: %s: The name of a menu. */
const actionLabel = __( "Switch to '%s'" );
Expand Down Expand Up @@ -54,40 +52,13 @@ const MainContent = ( {
[ clientId ]
);

const [ clientIdWithOpenLinkUI, setClientIdWithOpenLinkUI ] = useState();
const { lastInsertedBlockClientId } = useSelect( ( select ) => {
const { getLastInsertedBlocksClientIds } = unlock(
select( blockEditorStore )
);
const lastInsertedBlocksClientIds = getLastInsertedBlocksClientIds();
return {
lastInsertedBlockClientId:
lastInsertedBlocksClientIds && lastInsertedBlocksClientIds[ 0 ],
};
}, [] );
const { updateBlockAttributes } = useDispatch( blockEditorStore );

const {
insertedBlockAttributes,
insertedBlockName,
setInsertedBlockAttributes,
} = useInsertedBlock( lastInsertedBlockClientId );

const hasExistingLinkValue = insertedBlockAttributes?.url;

useEffect( () => {
if (
lastInsertedBlockClientId &&
BLOCKS_WITH_LINK_UI_SUPPORT?.includes( insertedBlockName ) &&
! hasExistingLinkValue // don't re-show the Link UI if the block already has a link value.
) {
setClientIdWithOpenLinkUI( lastInsertedBlockClientId );
}
}, [
lastInsertedBlockClientId,
clientId,
insertedBlockName,
hasExistingLinkValue,
] );
const setInsertedBlockAttributes =
( _insertedBlockClientId ) => ( _updatedAttributes ) => {
if ( ! _insertedBlockClientId ) return;
updateBlockAttributes( _insertedBlockClientId, _updatedAttributes );
};

const { navigationMenu } = useNavigationMenu( currentMenuId );

Expand All @@ -109,23 +80,45 @@ const MainContent = ( {
'You have not yet created any menus. Displaying a list of your Pages'
);

const renderLinkUI = ( block ) => {
const renderLinkUI = (
currentBlock,
lastInsertedBlock,
setLastInsertedBlock
) => {
const blockHasExistingLinkValue = !! lastInsertedBlock?.url;
const blockSupportsLinkUI = BLOCKS_WITH_LINK_UI_SUPPORT?.includes(
lastInsertedBlock?.name
);
const currentBlockWasJustInserted =
lastInsertedBlock?.clientId === currentBlock.clientId;

const shouldShowLinkUIForBlock =
blockSupportsLinkUI &&
! blockHasExistingLinkValue && // don't re-show the Link UI if the block already has a link value.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this check necessary? If it only appears after a user has inserted the block, it should never have a link, right? If so, I think we can remove all the checks for if there's a link or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeh you could be right there. Have you got time to test that and commit as it's nearly my EOD?

currentBlockWasJustInserted;

return (
clientIdWithOpenLinkUI === block.clientId && (
shouldShowLinkUIForBlock && (
<LinkUI
clientId={ lastInsertedBlockClientId }
link={ insertedBlockAttributes }
onClose={ () => setClientIdWithOpenLinkUI( null ) }
clientId={ lastInsertedBlock?.clientId }
link={ lastInsertedBlock?.attributes }
onClose={ () => {
setLastInsertedBlock( null );
} }
hasCreateSuggestion={ false }
onChange={ ( updatedValue ) => {
updateAttributes(
updatedValue,
setInsertedBlockAttributes,
insertedBlockAttributes
setInsertedBlockAttributes(
lastInsertedBlock?.clientId
),
lastInsertedBlock?.attributes
);
setClientIdWithOpenLinkUI( null );
setLastInsertedBlock( null );
} }
onCancel={ () => {
setLastInsertedBlock( null );
} }
onCancel={ () => setClientIdWithOpenLinkUI( null ) }
/>
)
);
Expand Down
Loading