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.

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 blue line indicator not showing at the end #25849

Merged
merged 6 commits into from
Oct 8, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import classnames from 'classnames';
/**
* WordPress dependencies
*/
import { createContext, useContext } from '@wordpress/element';
import { withSelect } from '@wordpress/data';
import { getDefaultBlockName } from '@wordpress/blocks';

Expand All @@ -16,6 +17,9 @@ import { getDefaultBlockName } from '@wordpress/blocks';
import DefaultBlockAppender from '../default-block-appender';
import ButtonBlockAppender from '../button-block-appender';

// A Context to store the map of the appender map.
export const AppenderNodesContext = createContext();

function stopPropagation( event ) {
event.stopPropagation();
}
Expand All @@ -30,6 +34,8 @@ function BlockListAppender( {
selectedBlockClientId,
tagName: TagName = 'div',
} ) {
const appenderNodesMap = useContext( AppenderNodesContext );

if ( isLocked || CustomAppender === false ) {
return null;
}
Expand Down Expand Up @@ -89,7 +95,20 @@ function BlockListAppender( {
// Prevent the block from being selected when the appender is
// clicked.
onFocus={ stopPropagation }
className={ classnames( 'block-list-appender', className ) }
className={ classnames(
'block-list-appender',
'wp-block',
className
) }
ref={ ( ref ) => {
if ( ref ) {
// Set the reference of the "Appender" with `rootClientId` as key.
appenderNodesMap.set( rootClientId || '', ref );
} else {
// If it un-mounts, cleanup the map.
appenderNodesMap.delete( rootClientId || '' );
}
} }
>
{ appender }
</TagName>
Expand Down
108 changes: 59 additions & 49 deletions packages/block-editor/src/components/block-list/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,17 @@ import { useRef, forwardRef } from '@wordpress/element';
* Internal dependencies
*/
import BlockListBlock from './block';
import BlockListAppender from '../block-list-appender';
import BlockListAppender, {
AppenderNodesContext,
} from '../block-list-appender';
import RootContainer from './root-container';
import useBlockDropZone from '../use-block-drop-zone';

/**
* A map to store the reference of each "Appenders" rendered with `rootClientId` as key.
*/
const appenderNodesMap = new Map();

/**
* If the block count exceeds the threshold, we disable the reordering animation
* to avoid laginess.
Expand Down Expand Up @@ -82,57 +89,60 @@ function BlockList(
dropTargetIndex === blockClientIds.length && isDraggingBlocks;

return (
<Container
{ ...__experimentalPassedProps }
ref={ element }
className={ classnames(
'block-editor-block-list__layout',
className,
__experimentalPassedProps.className
) }
>
{ blockClientIds.map( ( clientId, index ) => {
const isBlockInSelection = hasMultiSelection
? multiSelectedBlockClientIds.includes( clientId )
: selectedBlockClientId === clientId;
<AppenderNodesContext.Provider value={ appenderNodesMap }>
<Container
{ ...__experimentalPassedProps }
ref={ element }
className={ classnames(
'block-editor-block-list__layout',
className,
__experimentalPassedProps.className
) }
>
{ blockClientIds.map( ( clientId, index ) => {
const isBlockInSelection = hasMultiSelection
? multiSelectedBlockClientIds.includes( clientId )
: selectedBlockClientId === clientId;

const isDropTarget =
dropTargetIndex === index && isDraggingBlocks;
const isDropTarget =
dropTargetIndex === index && isDraggingBlocks;

return (
<AsyncModeProvider
key={ clientId }
value={ ! isBlockInSelection }
>
<BlockListBlock
rootClientId={ rootClientId }
clientId={ clientId }
// This prop is explicitely computed and passed down
// to avoid being impacted by the async mode
// otherwise there might be a small delay to trigger the animation.
index={ index }
enableAnimation={ enableAnimation }
className={ classnames( {
'is-drop-target': isDropTarget,
'is-dropping-horizontally':
isDropTarget &&
orientation === 'horizontal',
} ) }
/>
</AsyncModeProvider>
);
} ) }
<BlockListAppender
tagName={ __experimentalAppenderTagName }
rootClientId={ rootClientId }
renderAppender={ renderAppender }
className={ classnames( {
'is-drop-target': isAppenderDropTarget,
'is-dropping-horizontally':
isAppenderDropTarget && orientation === 'horizontal',
return (
<AsyncModeProvider
key={ clientId }
value={ ! isBlockInSelection }
>
<BlockListBlock
rootClientId={ rootClientId }
clientId={ clientId }
// This prop is explicitely computed and passed down
// to avoid being impacted by the async mode
// otherwise there might be a small delay to trigger the animation.
index={ index }
enableAnimation={ enableAnimation }
className={ classnames( {
'is-drop-target': isDropTarget,
'is-dropping-horizontally':
isDropTarget &&
orientation === 'horizontal',
} ) }
/>
</AsyncModeProvider>
);
} ) }
/>
</Container>
<BlockListAppender
tagName={ __experimentalAppenderTagName }
rootClientId={ rootClientId }
renderAppender={ renderAppender }
className={ classnames( {
'is-drop-target': isAppenderDropTarget,
'is-dropping-horizontally':
isAppenderDropTarget &&
orientation === 'horizontal',
} ) }
/>
</Container>
</AppenderNodesContext.Provider>
);
}

Expand Down
60 changes: 38 additions & 22 deletions packages/block-editor/src/components/block-list/insertion-point.js
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@ import classnames from 'classnames';
* WordPress dependencies
*/
import { useSelect } from '@wordpress/data';
import { useState, useRef } from '@wordpress/element';
import { useState, useRef, useMemo, useContext } from '@wordpress/element';
import { Popover } from '@wordpress/components';
import { placeCaretAtVerticalEdge } from '@wordpress/dom';

Expand All @@ -17,6 +17,7 @@ import { placeCaretAtVerticalEdge } from '@wordpress/dom';
import Inserter from '../inserter';
import { getClosestTabbable } from '../writing-flow';
import { getBlockDOMNode } from '../../utils/dom';
import { AppenderNodesContext } from '../block-list-appender';

function InsertionPointInserter( {
clientId,
Expand Down Expand Up @@ -98,13 +99,24 @@ function InsertionPointInserter( {

function InsertionPointPopover( {
clientId,
rootClientId,
isInserterShown,
isInserterForced,
setIsInserterForced,
containerRef,
showInsertionPoint,
} ) {
const element = getBlockDOMNode( clientId );
const appenderNodesMap = useContext( AppenderNodesContext );
const element = useMemo( () => {
if ( clientId ) {
return getBlockDOMNode( clientId );
}

// Can't find the element, might be at the end of the block list, or inside an empty block list.
// We instead try to find the "Appender" and place the indicator above it.
// `rootClientId` could be null or undefined when there's no parent block, we normalize it to an empty string.
return appenderNodesMap.get( rootClientId || '' );
Copy link
Member

Choose a reason for hiding this comment

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

The insertion point is added at the root of the whole tree of blocks, so I think rootClientId will always be '', never an id?

Copy link
Member

Choose a reason for hiding this comment

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

In other words, does the indicator work for nested appenders?

Copy link
Member Author

Choose a reason for hiding this comment

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

It could be an id if it's in the Widget Editor though, widget-area is kind of like a special case for this 😜.

Also, <InsertionPoint> only handles getting of the appender nodes. <BlockList> is where setting happens, which can be in <InnerBlocks> or widget-area. What I'm trying to say here is that a <InsertionPoint> could have multiple <BlockList> hence could also have multiple <BlockListAppender>.

Copy link
Member Author

Choose a reason for hiding this comment

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

In other words, does the indicator work for nested appenders?

It should, at least for my testing below.

image

}, [ clientId, rootClientId, appenderNodesMap ] );

return (
<Popover
Expand Down Expand Up @@ -139,26 +151,29 @@ export default function InsertionPoint( { children, containerRef } ) {
const [ isInserterShown, setIsInserterShown ] = useState( false );
const [ isInserterForced, setIsInserterForced ] = useState( false );
const [ inserterClientId, setInserterClientId ] = useState( null );
const { isMultiSelecting, isInserterVisible, selectedClientId } = useSelect(
( select ) => {
const {
isMultiSelecting: _isMultiSelecting,
isBlockInsertionPointVisible,
getBlockInsertionPoint,
getBlockOrder,
} = select( 'core/block-editor' );

const insertionPoint = getBlockInsertionPoint();
const order = getBlockOrder( insertionPoint.rootClientId );

return {
isMultiSelecting: _isMultiSelecting(),
isInserterVisible: isBlockInsertionPointVisible(),
selectedClientId: order[ insertionPoint.index ],
};
},
[]
);
const {
isMultiSelecting,
isInserterVisible,
selectedClientId,
selectedRootClientId,
} = useSelect( ( select ) => {
const {
isMultiSelecting: _isMultiSelecting,
isBlockInsertionPointVisible,
getBlockInsertionPoint,
getBlockOrder,
} = select( 'core/block-editor' );

const insertionPoint = getBlockInsertionPoint();
const order = getBlockOrder( insertionPoint.rootClientId );

return {
isMultiSelecting: _isMultiSelecting(),
isInserterVisible: isBlockInsertionPointVisible(),
selectedClientId: order[ insertionPoint.index ],
selectedRootClientId: insertionPoint.rootClientId,
};
}, [] );

function onMouseMove( event ) {
if (
Expand Down Expand Up @@ -215,6 +230,7 @@ export default function InsertionPoint( { children, containerRef } ) {
clientId={
isInserterVisible ? selectedClientId : inserterClientId
}
rootClientId={ selectedRootClientId }
isInserterShown={ isInserterShown }
isInserterForced={ isInserterForced }
setIsInserterForced={ setIsInserterForced }
Expand Down
4 changes: 4 additions & 0 deletions packages/block-editor/src/components/block-list/style.scss
Original file line number Diff line number Diff line change
Expand Up @@ -365,6 +365,10 @@
position: relative;
z-index: z-index(".block-editor-block-list__insertion-point");
margin-top: -$block-padding * 2;

&.is-insert-after {
margin-top: $block-padding;
}
}

.block-editor-block-list__insertion-point-indicator {
Expand Down
33 changes: 33 additions & 0 deletions packages/e2e-tests/specs/editor/various/adding-blocks.test.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@ import {
pressKeyTimes,
setBrowserViewport,
closeGlobalBlockInserter,
searchForBlock,
} from '@wordpress/e2e-test-utils';

/** @typedef {import('puppeteer').ElementHandle} ElementHandle */
Expand Down Expand Up @@ -262,4 +263,36 @@ describe( 'adding blocks', () => {
await coverBlock.click();
expect( await getEditedPostContent() ).toMatchSnapshot();
} );

// Check for regression of https://github.com/WordPress/gutenberg/issues/25785
it( 'inserts a block should show a blue line indicator', async () => {
// First insert a random Paragraph.
await insertBlock( 'Paragraph' );
await page.keyboard.type( 'First paragraph' );

// Open the global inserter and search for the Heading block.
await searchForBlock( 'Heading' );

const headingButton = (
await page.$x( `//button//span[contains(text(), 'Heading')]` )
)[ 0 ];

// Hover over the block should show the blue line indicator.
await headingButton.hover();

// Should show the blue line indicator somewhere.
const indicator = await page.$(
'.block-editor-block-list__insertion-point-indicator'
);
const indicatorRect = await indicator.boundingBox();

const paragraphBlock = await page.$(
'p[aria-label="Paragraph block"]'
);
const paragraphRect = await paragraphBlock.boundingBox();

// The blue line indicator should be below the last block.
expect( indicatorRect.x ).toBe( paragraphRect.x );
expect( indicatorRect.y > paragraphRect.y ).toBe( true );
} );
} );
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ export default function WidgetAreaInnerBlocks() {
onInput={ onInput }
onChange={ onChange }
templateLock={ false }
renderAppender={ InnerBlocks.DefaultBlockAppender }
/>
);
}