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

Try inserting blocks directly in empty grid cells #63108

Merged
merged 21 commits into from
Jul 9, 2024
Merged
Show file tree
Hide file tree
Changes from 12 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 @@ -18,14 +18,15 @@ import deprecated from '@wordpress/deprecated';
import Inserter from '../inserter';

function ButtonBlockAppender(
{ rootClientId, className, onFocus, tabIndex },
{ rootClientId, className, onFocus, tabIndex, onSelectCallback },
ref
) {
return (
<Inserter
position="bottom center"
rootClientId={ rootClientId }
__experimentalIsQuick
onSelectCallback={ onSelectCallback }
Copy link
Member

Choose a reason for hiding this comment

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

Is the Callback suffix necessary? The on prefix already hints that it's a function so I'd simply call this onSelect.

renderToggle={ ( {
onToggle,
disabled,
Expand Down
202 changes: 168 additions & 34 deletions packages/block-editor/src/components/grid/grid-visualizer.js
Original file line number Diff line number Diff line change
Expand Up @@ -15,9 +15,10 @@ import { __experimentalUseDropZone as useDropZone } from '@wordpress/compose';
*/
import { __unstableUseBlockElement as useBlockElement } from '../block-list/use-block-props/use-block-refs';
import BlockPopoverCover from '../block-popover/cover';
import { range, GridRect, getGridInfo } from './utils';
import { range, GridRect, getGridInfo, getComputedCSS } from './utils';
import { store as blockEditorStore } from '../../store';
import { useGetNumberOfBlocksBeforeCell } from './use-get-number-of-blocks-before-cell';
import ButtonBlockAppender from '../button-block-appender';

export function GridVisualizer( { clientId, contentRef, parentLayout } ) {
const isDistractionFree = useSelect(
Expand All @@ -44,6 +45,28 @@ export function GridVisualizer( { clientId, contentRef, parentLayout } ) {
);
}

const checkIfCellOccupied = ( gridElement, column, row ) => {
const cell = Array.from( gridElement.children ).find( ( child ) => {
Copy link
Member

Choose a reason for hiding this comment

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

Bit worried about performance here. We're iterating through the grid's children once per cell. So for a 16x16 grid with blocks in every cell that's 16*16*16*16 calls to getComputedCSS on every render.

An alternative approach might be to calculate rects similar to use-grid-layout-sync.js:50 and then you can tell if a cell is occupied by looking for a rect where rect.contains( row, column ).

I might try that if you don't mind me pushing to this branch.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure, go ahead!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the bug you're seeing with appender still appearing when a paragraph is added must happening in this function, because the gridElement refreshes with the new paragraph and its correct col and row attributes, but isCellOccupied still returns false for that cell.

Copy link
Member

Choose a reason for hiding this comment

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

How's this look? 9a626e3

Not very scientific but it feels a bit snappier to me clicking around.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmmm, I'm seeing The 'useSelect' hook returns different values when called with the same state and parameters. This can lead to unnecessary rerenders. in the console when selecting the grid.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But the issue with the appender still rendering is fixed! And the code looks much neater ☺️

Copy link
Member

Choose a reason for hiding this comment

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

Oops. Fixed.

const [ columnStart, columnSpan ] =
getComputedCSS( child, 'grid-column' ).match( /\d+/g ) || [];
const [ rowStart, rowSpan ] =
getComputedCSS( child, 'grid-row' ).match( /\d+/g ) || [];
const columnEnd = columnSpan
? parseInt( columnStart, 10 ) + parseInt( columnSpan, 10 ) - 1
: columnStart;
const rowEnd = rowSpan
? parseInt( rowStart, 10 ) + parseInt( rowSpan, 10 ) - 1
: rowStart;
return (
column >= columnStart &&
column <= columnEnd &&
row >= rowStart &&
row <= rowEnd
);
} );
return cell !== undefined;
};

const GridVisualizerGrid = forwardRef(
( { clientId, gridElement, isManualGrid }, ref ) => {
const [ gridInfo, setGridInfo ] = useState( () =>
Expand All @@ -52,6 +75,17 @@ const GridVisualizerGrid = forwardRef(
const [ isDroppingAllowed, setIsDroppingAllowed ] = useState( false );
const [ highlightedRect, setHighlightedRect ] = useState( null );

const {
updateBlockAttributes,
moveBlocksToPosition,
__unstableMarkNextChangeAsNotPersistent,
} = useDispatch( blockEditorStore );

const getNumberOfBlocksBeforeCell = useGetNumberOfBlocksBeforeCell(
clientId,
gridInfo.numColumns
);

useEffect( () => {
const observers = [];
for ( const element of [ gridElement, ...gridElement.children ] ) {
Expand Down Expand Up @@ -99,25 +133,84 @@ const GridVisualizerGrid = forwardRef(
{ isManualGrid
? range( 1, gridInfo.numRows ).map( ( row ) =>
range( 1, gridInfo.numColumns ).map(
( column ) => (
<GridVisualizerCell
key={ `${ row }-${ column }` }
color={ gridInfo.currentColor }
>
<GridVisualizerDropZone
column={ column }
row={ row }
gridClientId={ clientId }
gridInfo={ gridInfo }
highlightedRect={
highlightedRect
( column ) => {
const isCellOccupied =
checkIfCellOccupied(
gridElement,
column,
row
);
const isHighlighted =
highlightedRect?.contains(
column,
row
) ?? false;
return (
<GridVisualizerCell
key={ `${ row }-${ column }` }
color={ gridInfo.currentColor }
className={
isHighlighted &&
'is-highlighted'
}
setHighlightedRect={
setHighlightedRect
}
/>
</GridVisualizerCell>
)
>
{ isCellOccupied ? (
<GridVisualizerDropZone
column={ column }
row={ row }
gridClientId={
clientId
}
gridInfo={ gridInfo }
setHighlightedRect={
setHighlightedRect
}
/>
) : (
<GridVisualizerAppender
column={ column }
row={ row }
gridClientId={
clientId
}
gridInfo={ gridInfo }
setHighlightedRect={
setHighlightedRect
}
onSelectCallback={ (
block
) => {
updateBlockAttributes(
block.clientId,
{
style: {
layout: {
columnStart:
column,
rowStart:
row,
},
},
}
);
__unstableMarkNextChangeAsNotPersistent();
moveBlocksToPosition(
[
block.clientId,
],
clientId,
clientId,
getNumberOfBlocksBeforeCell(
column,
row
)
);
} }
Copy link
Member

Choose a reason for hiding this comment

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

I'd move this callback logic into GridVisualizerAppender so that it's more consistent with GridVisualizerDropZone (which does all of that logic in the component) and so that this code isn't so heavily indented.

/>
) }
</GridVisualizerCell>
);
}
)
)
: Array.from(
Expand All @@ -135,28 +228,32 @@ const GridVisualizerGrid = forwardRef(
}
);

function GridVisualizerCell( { color, children } ) {
function GridVisualizerCell( { color, children, className } ) {
return (
<div
className="block-editor-grid-visualizer__cell"
className={ clsx(
'block-editor-grid-visualizer__cell',
className
) }
style={ {
boxShadow: `inset 0 0 0 1px color-mix(in srgb, ${ color } 20%, #0000)`,
color,
} }
>
{ children }
</div>
);
}

function GridVisualizerDropZone( {
function useGridVisualizerDropZone(
column,
row,
gridClientId,
gridInfo,
highlightedRect,
setHighlightedRect,
} ) {
const { getBlockAttributes } = useSelect( blockEditorStore );
setHighlightedRect
) {
const { getBlockAttributes, getBlockRootClientId } =
useSelect( blockEditorStore );
const {
updateBlockAttributes,
moveBlocksToPosition,
Expand All @@ -168,7 +265,7 @@ function GridVisualizerDropZone( {
gridInfo.numColumns
);

const ref = useDropZoneWithValidation( {
return useDropZoneWithValidation( {
validateDrag( srcClientId ) {
const attributes = getBlockAttributes( srcClientId );
const rect = new GridRect( {
Expand Down Expand Up @@ -221,21 +318,58 @@ function GridVisualizerDropZone( {
__unstableMarkNextChangeAsNotPersistent();
moveBlocksToPosition(
[ srcClientId ],
gridClientId,
getBlockRootClientId( srcClientId ),
Copy link
Member

Choose a reason for hiding this comment

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

What's this change for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We need to allow blocks from outside the grid (so with a different rootClientId) to be dropped inside the grid too.

gridClientId,
getNumberOfBlocksBeforeCell( column, row )
);
},
} );
}

const isHighlighted = highlightedRect?.contains( column, row ) ?? false;

function GridVisualizerDropZone( {
column,
row,
gridClientId,
gridInfo,
setHighlightedRect,
} ) {
return (
<div
ref={ ref }
className={ clsx( 'block-editor-grid-visualizer__drop-zone', {
'is-highlighted': isHighlighted,
} ) }
className="block-editor-grid-visualizer__drop-zone"
ref={ useGridVisualizerDropZone(
column,
row,
gridClientId,
gridInfo,
setHighlightedRect
) }
/>
);
}

function GridVisualizerAppender( {
column,
row,
gridClientId,
gridInfo,
setHighlightedRect,
onSelectCallback,
} ) {
return (
<ButtonBlockAppender
rootClientId={ gridClientId }
className="block-editor-grid-visualizer__appender"
ref={ useGridVisualizerDropZone(
column,
row,
gridClientId,
gridInfo,
setHighlightedRect
) }
style={ {
color: gridInfo.currentColor,
} }
onSelectCallback={ onSelectCallback }
/>
);
}
Expand Down
50 changes: 43 additions & 7 deletions packages/block-editor/src/components/grid/style.scss
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,9 @@
pointer-events: all;
}
}
.block-editor-inserter * {
pointer-events: auto;
}
}
}

Expand All @@ -20,23 +23,55 @@
}

.block-editor-grid-visualizer__cell {
align-items: center;
display: flex;
justify-content: center;
display: grid;
position: relative;

.block-editor-inserter {
color: inherit;
z-index: 32;
position: absolute;
top: 0;
bottom: 0;
left: 0;
right: 0;
overflow: hidden;

.block-editor-grid-visualizer__appender {
box-shadow: inset 0 0 0 1px color-mix(in srgb, currentColor 20%, #0000);
color: inherit;
overflow: hidden;
height: 100%;
width: 100%;
padding: 0 !important;
opacity: 0;
}

}

&.is-highlighted {
.block-editor-inserter,
.block-editor-grid-visualizer__drop-zone {
background: var(--wp-admin-theme-color);
}
}

&:hover .block-editor-grid-visualizer__appender,
.block-editor-grid-visualizer__appender:focus {
opacity: 1;
background-color: color-mix(in srgb, currentColor 20%, #0000);
}
}

.block-editor-grid-visualizer__drop-zone {
background: rgba($gray-400, 0.1);
width: 100%;
height: 100%;
grid-column: 1;
grid-row: 1;

// Make drop zone 8x8 at minimum so that it's easier to drag into. This will overflow the parent.
min-width: $grid-unit-10;
min-height: $grid-unit-10;

&.is-highlighted {
background: var(--wp-admin-theme-color);
}
}

.block-editor-grid-item-resizer {
Expand All @@ -60,3 +95,4 @@
}
}
}

4 changes: 4 additions & 0 deletions packages/block-editor/src/components/inserter/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,7 @@ class PrivateInserter extends Component {
prioritizePatterns,
onSelectOrClose,
selectBlockOnInsert,
onSelectCallback,
Copy link
Member

Choose a reason for hiding this comment

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

This component has onSelectOrClose and onSelectCallback. It should only need one.

} = this.props;

if ( isQuick ) {
Expand All @@ -167,6 +168,9 @@ class PrivateInserter extends Component {
onSelectOrClose( firstBlock );
}
onClose();
if ( onSelectCallback ) {
onSelectCallback( firstBlock );
}
} }
rootClientId={ rootClientId }
clientId={ clientId }
Expand Down
Loading
Loading