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

Show all blocks all the times to avoid confusing UX #61873

Closed
wants to merge 22 commits into from
Closed

Show all blocks all the times to avoid confusing UX #61873

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
dec05da
add a new panel to show all blocks when we have too few blocks
draganescu May 22, 2024
744f623
Insert blocks from general list at the end of the canvas
jeryj May 22, 2024
0d4a803
Put logic for finding the insertion point within useInsertionPoint
jeryj May 22, 2024
4455c10
insert the block at the immediately available next index
draganescu May 23, 2024
97155f9
enable searching through blocks
draganescu May 23, 2024
8de5ca8
Revert separating out createNewBlock
jeryj May 23, 2024
df9f4f2
Reuse getBlockRootClientId from upper scope
jeryj May 23, 2024
071f067
Check all blocks to see if they can insert, abort if root
jeryj May 23, 2024
36d7125
Use Allowed for drilldown category title. Output all items as grid tr…
jeryj May 23, 2024
7145856
Allow inserting blocks to post-content block on page
jeryj May 23, 2024
c2fbc8f
make finding inserter blocks for template locked independent of block…
draganescu May 24, 2024
a8b51dc
remove block name check and search top down for places to insert if a…
draganescu May 28, 2024
23ae559
Remove unused comment
jeryj May 28, 2024
19c8abc
Try using block insertion point to share state of block insertion acr…
jeryj May 29, 2024
cd3f029
Reorder output of array items for useBlockTypesState to preserve prev…
jeryj May 30, 2024
3db4d5f
If using the appender, don't use blockInsertionPoint
jeryj May 30, 2024
e1827aa
Fix direct insert via appender
jeryj May 30, 2024
c9bbc41
Fix columns block test
jeryj May 30, 2024
202875d
Fix unit tests for showInsertionPoint
jeryj May 30, 2024
b1775a7
Use root block title when only one category
jeryj May 30, 2024
27379f2
Fix mobile test by mocking getBlocks
jeryj May 30, 2024
e19ab3e
Use Block Title + Block for category name
jeryj May 30, 2024
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
83 changes: 74 additions & 9 deletions packages/block-editor/src/components/inserter/block-types-tab.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
/**
* WordPress dependencies
*/
import { __, _x } from '@wordpress/i18n';
import { __, _x, sprintf } from '@wordpress/i18n';
import { useMemo, useEffect, forwardRef } from '@wordpress/element';
import { pipe, useAsyncList } from '@wordpress/compose';

Expand All @@ -14,6 +14,8 @@ import useBlockTypesState from './hooks/use-block-types-state';
import InserterListbox from '../inserter-listbox';
import { orderBy } from '../../utils/sorting';
import InserterNoResults from './no-results';
import { select } from '@wordpress/data';
import { store as blockEditorStore } from '../../store';

const getBlockNamespace = ( item ) => item.name.split( '/' )[ 0 ];

Expand All @@ -31,10 +33,27 @@ export function BlockTypesTab(
{ rootClientId, onInsert, onHover, showMostUsedBlocks },
ref
) {
const [ items, categories, collections, onSelectItem ] = useBlockTypesState(
rootClientId,
onInsert
);
const { getBlockName } = select( blockEditorStore );
const [ items, categories, collections, onSelectItem, allItems ] =
useBlockTypesState( rootClientId, onInsert );

const allItemsPerCategory = useMemo( () => {
return pipe(
( itemList ) =>
itemList.filter(
( item ) => item.category && item.category !== 'reusable'
),
( itemList ) =>
itemList.reduce( ( acc, item ) => {
Copy link
Member

Choose a reason for hiding this comment

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

Why loop over this twice and add pipe if it can be done in a single loop?

const { category } = item;
if ( ! acc[ category ] ) {
acc[ category ] = [];
}
acc[ category ].push( item );
return acc;
}, {} )
)( allItems );
}, [ allItems ] );

const suggestedItems = useMemo( () => {
return orderBy( items, 'frecency', 'desc' ).slice(
Expand Down Expand Up @@ -83,6 +102,19 @@ export function BlockTypesTab(
// Hide block preview on unmount.
useEffect( () => () => onHover( null ), [] );

const getRootBlockTitle = () => {
const blockName = getBlockName( rootClientId );
// Find the title within the list of all possible blocks.
const block = allItems.find( ( item ) => item.name === blockName );
Copy link
Member

Choose a reason for hiding this comment

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

Are we getting the block title here? Why not check the block type instead of looping through all blocks, like we normally do?

return block
? sprintf(
// translators: %s: Block title, e.g: "List", "Buttons", "Group"
__( '%s Block' ),
Copy link
Member

Choose a reason for hiding this comment

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

We don't need "Block" appended here. The block name itself will be enough context. "List", "Columns", "Buttons", etc.

block.title
)
: __( 'Allowed' );
};

/**
* The inserter contains a big number of blocks and opening it is a costful operation.
* The rendering is the most costful part of it, in order to improve the responsiveness
Expand All @@ -101,13 +133,18 @@ export function BlockTypesTab(
didRenderAllCategories ? collectionEntries : EMPTY_ARRAY
);

if ( ! items.length ) {
return <InserterNoResults />;
}
const availableCategories = categories.filter( ( category ) => {
const categoryItems = itemsPerCategory[ category.slug ];
if ( ! categoryItems || ! categoryItems.length ) {
return false;
}
return category;
} );

return (
<InserterListbox>
<div ref={ ref }>
{ ! items.length && ! allItems.length && <InserterNoResults /> }
{ showMostUsedBlocks && !! suggestedItems.length && (
<InserterPanel title={ _x( 'Most used', 'blocks' ) }>
<BlockTypesList
Expand All @@ -127,7 +164,11 @@ export function BlockTypesTab(
return (
<InserterPanel
key={ category.slug }
title={ category.title }
title={
categoryItems.length === 1
? getRootBlockTitle()
: category.title
Copy link
Member

Choose a reason for hiding this comment

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

I don't understand this condition. Why do we fall back to the title of the parent of the selected block when there's 1 category?

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I got confused by categoryItems, I'm guessing this is just the total amount of items, not the amount of categories.

}
icon={ category.icon }
>
<BlockTypesList
Expand Down Expand Up @@ -177,6 +218,30 @@ export function BlockTypesTab(
);
}
) }

{ ( items.length === 0 ||
availableCategories.length === 1 ) && (
Copy link
Member

Choose a reason for hiding this comment

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

Would be great to comment on this condition. Why show this when there's 1 category available and not 2?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Why show this when there's 1 category available and not 2?

Pure convention 🤷🏻

<div className="block-editor-inserter__all-blocks">
{ categories.map( ( category ) => {
const categoryItems =
allItemsPerCategory[ category.slug ];
return (
<InserterPanel
key={ category.slug }
title={ category.title }
icon={ category.icon }
>
<BlockTypesList
items={ categoryItems }
onSelect={ onSelectItem }
onHover={ onHover }
label={ category.title }
/>
</InserterPanel>
);
} ) }
</div>
) }
</div>
</InserterListbox>
);
Expand Down
22 changes: 21 additions & 1 deletion packages/block-editor/src/components/inserter/hooks/use-block-types-state.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,25 @@ import { store as blockEditorStore } from '../../../store';
* @return {Array} Returns the block types state. (block types, categories, collections, onSelect handler)
*/
const useBlockTypesState = ( rootClientId, onInsert ) => {
const [ allItems ] = useSelect( ( select ) => {
let availableItems = select( blockEditorStore ).getInserterItems( '' );

// use current selection as root for situations like
// template locked mode
const rootBlocks = select( blockEditorStore ).getBlocks();
while ( availableItems.length === 0 ) {
for ( const block of rootBlocks ) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This loop feels like it can be very bad perf wise in nested templates and things like that. (not sure how true though).

The other thing that feels odd, is that it will end up with a random client id and the behavior for the user can be confusing: where am I going to insert when I click that thing?

It feels like a better path, would be to try to find the "rootClientId" that we want to insert in before hand and update useBlockTypesState to take a rootClientId as an argument. Now that I'm writing that, it seems it already has a rootClientId, so it's a matter of extracting a "BlockList" component from the current BlockTab component and doing something like:

const { rootClientId, alternativeRootClientId } = useSelect(() => {
   return {
       rootClientId: getInsertionPoint().rootClientId,
       alternativeRootClientId: findAlternativeRootClientId() // we can discuss implementation of this function separately.
   }
}, [] );

<BlockList rootClientId={ rootClientID } />
<BlockList rootClientId={ alternativeRootClientId } />

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes by all means there a couple of risky loops but they're in place just to make things "work" and to show conceptual limitations we still have.

For example a loop later in the PR will make the action of inserting a block while a post content block is selected in a template locked edit mode and it shows that we can't tell where to insert without:

  • bringing knowledge of template locked mode to the block editor package
  • relying on something else in a higher level package finding "post content" and its client id and pass that down to lower levels

... both of which are kinda weird.

Separately, the theoretical findAlternativeRootClientId will also have to try a sort of heuristic since we don't have a clear picture of how to find the next best insertable spot.

Maybe the block list should advertise this in a separate precomputed part of the store? As in top down places to insert blocks if the next thing is unavailable ... or something.

Copy link
Contributor

Choose a reason for hiding this comment

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

There are two separate discussions here:

  • First; I don't think the way this PR is implemented is correct. It's just adding code to make things work as it want but without organizing things properly. This is for instance forcing us to have this loop in two places (find the right place).
  • Second: how findAlternativeRootClientId should be implemented. There are options: smart as the PR is trying to do (find the closest parent or something), or more controlled: (use the same wrapper the zoom-out would use for instance)

Copy link
Contributor Author

@draganescu draganescu May 29, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I don't think the way this PR is implemented is correct. It's just adding code to make things work as it want but without organizing things properly

Yes, that's why I said: "by all means there a couple of risky loops but they're in place just to make things "work" and to show conceptual limitations we still have." and @jeryj said "We have gotten it to work, but it needs refactoring to be more stable and easier to maintain." 😂

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, sorry I misread the original reply :)

availableItems = select( blockEditorStore ).getInserterItems(
block.clientId
);
if ( availableItems.length ) {
break;
}
}
}
return [ availableItems ];
} );

const [ items ] = useSelect(
( select ) => [
select( blockEditorStore ).getInserterItems( rootClientId ),
Expand All @@ -32,6 +51,7 @@ const useBlockTypesState = ( rootClientId, onInsert ) => {

const [ categories, collections ] = useSelect( ( select ) => {
const { getCategories, getCollections } = select( blocksStore );

return [ getCategories(), getCollections() ];
}, [] );

Expand All @@ -56,7 +76,7 @@ const useBlockTypesState = ( rootClientId, onInsert ) => {
[ onInsert ]
);

return [ items, categories, collections, onSelectItem ];
return [ items, categories, collections, onSelectItem, allItems ];
};

export default useBlockTypesState;
42 changes: 34 additions & 8 deletions packages/block-editor/src/components/inserter/hooks/use-insertion-point.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,21 +41,29 @@ function useInsertionPoint( {
onSelect,
shouldFocusBlock = true,
selectBlockOnInsert = true,
blockToInsert,
} ) {
const { getSelectedBlock } = useSelect( blockEditorStore );
const { destinationRootClientId, destinationIndex } = useSelect(
const {
destinationRootClientId,
destinationIndex,
blockInsertionRootClientId,
blockInsertionIndex,
} = useSelect(
( select ) => {
const {
getSelectedBlockClientId,
getBlockRootClientId,
getBlockIndex,
getBlockOrder,
getBlockIndex,
getBlockRootClientId,
getBlockInsertionPoint,
} = select( blockEditorStore );
const selectedBlockClientId = getSelectedBlockClientId();

let _destinationRootClientId = rootClientId;
let _destinationIndex;

const blockInsertionPoint = getBlockInsertionPoint();
if ( insertionIndex !== undefined ) {
// Insert into a specific index.
_destinationIndex = insertionIndex;
Expand All @@ -74,9 +82,23 @@ function useInsertionPoint( {
).length;
}

// When we're using the appender or an insertion index has been passed directly,
// we want to use that over our "best guess" block insertion point
const isDirectInsert = insertionIndex !== undefined || isAppender;
const shouldUseBlockInsertionPoint =
! isDirectInsert &&
blockInsertionPoint?.rootClientId !== undefined &&
blockInsertionPoint?.index !== undefined;

return {
destinationRootClientId: _destinationRootClientId,
destinationIndex: _destinationIndex,
blockInsertionRootClientId: shouldUseBlockInsertionPoint
? blockInsertionPoint.rootClientId
: _destinationRootClientId,
blockInsertionIndex: shouldUseBlockInsertionPoint
? blockInsertionPoint.index
: _destinationIndex,
};
},
[ rootClientId, insertionIndex, clientId, isAppender ]
Expand Down Expand Up @@ -121,8 +143,8 @@ function useInsertionPoint( {
} else {
insertBlocks(
blocks,
destinationIndex,
destinationRootClientId,
blockInsertionIndex,
blockInsertionRootClientId,
selectBlockOnInsert,
shouldFocusBlock || shouldForceFocusBlock ? 0 : null,
meta
Expand All @@ -145,18 +167,21 @@ function useInsertionPoint( {
getSelectedBlock,
replaceBlocks,
insertBlocks,
destinationRootClientId,
destinationIndex,
blockInsertionRootClientId,
blockInsertionIndex,
onSelect,
shouldFocusBlock,
selectBlockOnInsert,
setLastFocus,
]
);

const onToggleInsertionPoint = useCallback(
( show ) => {
if ( show ) {
showInsertionPoint( destinationRootClientId, destinationIndex );
showInsertionPoint( destinationRootClientId, destinationIndex, {
block: blockToInsert,
Copy link
Member

Choose a reason for hiding this comment

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

I assume we need to pass the block so that we can change the target if necessary. Can't be change destinationRootClientId and destinationIndex so we avoid modifying showInsertionPoint?

} );
} else {
hideInsertionPoint();
}
Expand All @@ -166,6 +191,7 @@ function useInsertionPoint( {
hideInsertionPoint,
destinationRootClientId,
destinationIndex,
blockToInsert,
]
);

Expand Down
9 changes: 7 additions & 2 deletions packages/block-editor/src/components/inserter/menu.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import {
useCallback,
useMemo,
useRef,
useEffect,
useLayoutEffect,
} from '@wordpress/element';
import { VisuallyHidden, SearchControl, Popover } from '@wordpress/components';
Expand Down Expand Up @@ -77,6 +78,7 @@ function InserterMenu(
isAppender,
insertionIndex: __experimentalInsertionIndex,
shouldFocusBlock,
blockToInsert: hoveredItem,
} );
const blockTypesTabRef = useRef();

Expand Down Expand Up @@ -111,12 +113,15 @@ function InserterMenu(

const onHover = useCallback(
( item ) => {
onToggleInsertionPoint( !! item );
setHoveredItem( item );
},
[ onToggleInsertionPoint, setHoveredItem ]
[ setHoveredItem ]
);

useEffect( () => {
onToggleInsertionPoint( !! hoveredItem );
}, [ onToggleInsertionPoint, hoveredItem ] );

const onHoverPattern = useCallback(
( item ) => {
onToggleInsertionPoint( !! item );
Expand Down
6 changes: 5 additions & 1 deletion packages/block-editor/src/components/inserter/search-results.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,16 +76,20 @@ function InserterSearchResults( {
selectBlockOnInsert,
} );
const [
blockTypes,
availableBlockTypes,
blockTypeCategories,
blockTypeCollections,
onSelectBlockType,
allBlockTypes,
] = useBlockTypesState( destinationRootClientId, onInsertBlocks );
const [ patterns, , onClickPattern ] = usePatternsState(
onInsertBlocks,
destinationRootClientId
);

const blockTypes =
availableBlockTypes.length < 2 ? allBlockTypes : availableBlockTypes;

const filteredBlockPatterns = useMemo( () => {
if ( maxBlockPatterns === 0 ) {
return [];
Expand Down
4 changes: 4 additions & 0 deletions packages/block-editor/src/components/inserter/style.scss
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -200,6 +200,10 @@ $block-inserter-tabs-height: 44px;
width: 100%;
}

.block-editor-inserter__all-blocks {
border-top: $border-width solid $gray-400;
}

.block-editor-inserter__no-results,
.block-editor-inserter__patterns-loading {
padding: $grid-unit-40;
Expand Down
1 change: 1 addition & 0 deletions packages/block-editor/src/components/inserter/test/block-types-tab.native.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ const selectMock = {
getBlockType: jest.fn(),
getClipboard: jest.fn(),
getSettings: jest.fn( () => ( { impressions: {} } ) ),
getBlocks: jest.fn().mockReturnValue( [] ),
};

describe( 'BlockTypesTab component', () => {
Expand Down
Loading
Loading