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

Fix drag and drop indicator above first block and RTL drop indicators #33024

Merged
merged 4 commits into from
Jun 28, 2021
Merged

Fix drag and drop indicator above first block and RTL drop indicators #33024

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
855588a
Update logic
talldan Jun 28, 2021
7f4c072
RTL and block appender fixes
talldan Jun 28, 2021
77cae45
Fix RTL positioning calculations
talldan Jun 28, 2021
3394b7c
Avoid adding wp-block to the appender
talldan Jun 28, 2021
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
6 changes: 1 addition & 5 deletions packages/block-editor/src/components/block-list-appender/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -94,11 +94,7 @@ function BlockListAppender( {
// Prevent the block from being selected when the appender is
// clicked.
onFocus={ stopPropagation }
className={ classnames(
'block-list-appender',
'wp-block',
className
) }
className={ classnames( 'block-list-appender', className ) }
Copy link
Contributor Author

@talldan talldan Jun 28, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

wp-block was added here in #25849, but I don't think it should be here. Everything still works without it.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, it should be a mistake 😓

Copy link
Member

Choose a reason for hiding this comment

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

Why shouldn't it be there? Won't it give correct margins to the appender?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Didn't seem to make a difference from what I can tell.

Copy link
Contributor

Choose a reason for hiding this comment

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

It does make a difference, we should restore it, without it, the block appender won't be considered a "block" meaning it won't be aligned properly in a block list.

For instance you can test this by adding an image block and notice that the appender that shows after the image is not centered properly.

>
{ appender }
</TagName>
Expand Down
73 changes: 44 additions & 29 deletions packages/block-editor/src/components/block-tools/insertion-point.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,86 +79,101 @@ function InsertionPointPopover( {
}, [] );
const previousElement = useBlockElement( previousClientId );
const nextElement = useBlockElement( nextClientId );

const style = useMemo( () => {
if ( ! previousElement ) {
if ( ! previousElement && ! nextElement ) {
return {};
}
const previousRect = previousElement.getBoundingClientRect();

const previousRect = previousElement
? previousElement.getBoundingClientRect()
: null;
const nextRect = nextElement
? nextElement.getBoundingClientRect()
: null;

if ( orientation === 'vertical' ) {
return {
width: previousElement.offsetWidth,
height: nextRect ? nextRect.top - previousRect.bottom : 0,
width: previousElement
? previousElement.offsetWidth
: nextElement.offsetWidth,
height:
nextRect && previousRect
? nextRect.top - previousRect.bottom
: 0,
};
}

let width = 0;
if ( nextElement ) {
if ( previousRect && nextRect ) {
width = isRTL()
? previousRect.left - nextRect.right
: nextRect.left - previousRect.right;
}

return {
width,
height: previousElement.offsetHeight,
height: previousElement
? previousElement.offsetHeight
: nextElement.offsetHeight,
};
}, [ previousElement, nextElement ] );

const getAnchorRect = useCallback( () => {
const { ownerDocument } = previousElement;
const previousRect = previousElement.getBoundingClientRect();
if ( ! previousElement && ! nextElement ) {
return {};
}

const { ownerDocument } = previousElement || nextElement;

const previousRect = previousElement
? previousElement.getBoundingClientRect()
: null;
const nextRect = nextElement
? nextElement.getBoundingClientRect()
: null;

if ( orientation === 'vertical' ) {
if ( isRTL() ) {
return {
top: previousRect.bottom,
left: previousRect.right,
right: previousRect.left,
top: previousRect ? previousRect.bottom : nextRect.top,
left: previousRect ? previousRect.right : nextRect.right,
right: previousRect ? previousRect.left : nextRect.left,
bottom: nextRect ? nextRect.top : previousRect.bottom,
ownerDocument,
};
}

return {
top: previousRect.bottom,
left: previousRect.left,
right: previousRect.right,
top: previousRect ? previousRect.bottom : nextRect.top,
left: previousRect ? previousRect.left : nextRect.left,
right: previousRect ? previousRect.right : nextRect.right,
bottom: nextRect ? nextRect.top : previousRect.bottom,
ownerDocument,
};
Comment on lines 138 to 153
Copy link
Contributor

@adamziel adamziel Jun 28, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

This works pretty well but it got quite repetitive. How about something like:

const rect = {
	top: previousRect ? previousRect.bottom : nextRect.top,
	bottom: nextRect ? nextRect.top : previousRect.bottom,
	ownerDocument
};
if ( isRTL() ) {
	return {
		...rect,
		left: previousRect ? previousRect.right : nextRect.right,
		right: previousRect ? previousRect.left : nextRect.left,
	};
}
return {
	...rect,
	left: previousRect ? previousRect.left : nextRect.left,
	right: previousRect ? previousRect.right : nextRect.right,
};

Or perhaps even:

const [beforePrevRect, afterPrevRect] = isRTL() ? [previousRect.left, previousRect.right] : [previousRect.right, previousRect.left];
const [beforeNextRect, afterNextRect] = isRTL() ? [nextRect.left, nextRect.right] : [nextRect.right, nextRect.left];
return {
	top: previousRect ? previousRect.bottom : nextRect.top,
	bottom: nextRect ? nextRect.top : previousRect.bottom,
	left: previousRect ? beforePrevRect : beforeNextRect,
	right: previousRect ? afterPrevRect : afterNextRect,
	ownerDocument
};

Or even:

const [beforePrevRect, afterPrevRect] = isRTL() ? [previousRect.left, previousRect.right] : [previousRect.right, previousRect.left];
const [beforeNextRect, afterNextRect] = isRTL() ? [nextRect.left, nextRect.right] : [nextRect.right, nextRect.left];
const bottom = nextRect ? nextRect.top : previousRect.bottom;
const [left, right, top] = previousRect ? [beforePrevRect, afterPrevRect, previousRect.bottom] : [beforeNextRect, afterNextRect, netRect.top];
return { left, right, top, bottom, ownerDocument };

Copy link
Contributor

Choose a reason for hiding this comment

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

Anyway that's just a note about code style, feel free to decide to keep the current one or use one of the ideas above. I'm approving this PR regardless.

}

if ( isRTL() ) {
return {
top: previousRect.top,
left: nextRect ? nextRect.right : previousRect.left,
right: previousRect.left,
bottom: previousRect.bottom,
top: previousRect ? previousRect.top : nextRect.top,
left: previousRect ? previousRect.left : nextRect.right,
right: nextRect ? nextRect.right : previousRect.left,
bottom: previousRect ? previousRect.bottom : nextRect.bottom,
ownerDocument,
};
}

return {
top: previousRect.top,
left: previousRect.right,
top: previousRect ? previousRect.top : nextRect.top,
left: previousRect ? previousRect.right : nextRect.left,
right: nextRect ? nextRect.left : previousRect.right,
bottom: previousRect.bottom,
bottom: previousRect ? previousRect.bottom : nextRect.bottom,
ownerDocument,
};
}, [ previousElement, nextElement ] );

const popoverScrollRef = usePopoverScroll( __unstableContentRef );

if ( ! previousElement ) {
return null;
}

const className = classnames(
'block-editor-block-list__insertion-point',
'is-' + orientation
Expand All @@ -178,10 +193,10 @@ function InsertionPointPopover( {
}
}

// Only show the inserter when there's a `nextElement` (a block after the
// insertion point). At the end of the block list the trailing appender
// should serve the purpose of inserting blocks.
const showInsertionPointInserter = nextElement && isInserterShown;
// Only show the in-between inserter between blocks, so when there's a
// previous and a next element.
const showInsertionPointInserter =
previousElement && nextElement && isInserterShown;
Copy link
Member

Choose a reason for hiding this comment

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

isInserterShown might suffice here. The component controls it is the in between inserter hook showing or hiding the inserter.


/* eslint-disable jsx-a11y/no-static-element-interactions, jsx-a11y/click-events-have-key-events */
// While ideally it would be enough to capture the
Expand Down
10 changes: 9 additions & 1 deletion packages/block-editor/src/components/use-block-drop-zone/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import {
useThrottle,
__experimentalUseDropZone as useDropZone,
} from '@wordpress/compose';
import { isRTL } from '@wordpress/i18n';

/**
* Internal dependencies
Expand Down Expand Up @@ -39,6 +40,8 @@ export function getNearestBlockIndex( elements, position, orientation ) {
? [ 'left', 'right' ]
: [ 'top', 'bottom' ];

const isRightToLeft = isRTL();

let candidateIndex;
let candidateDistance;

Expand All @@ -53,7 +56,12 @@ export function getNearestBlockIndex( elements, position, orientation ) {
if ( candidateDistance === undefined || distance < candidateDistance ) {
// If the user is dropping to the trailing edge of the block
// add 1 to the index to represent dragging after.
const isTrailingEdge = edge === 'bottom' || edge === 'right';
// Take RTL languages into account where the left edge is
// the trailing edge.
const isTrailingEdge =
edge === 'bottom' ||
( ! isRightToLeft && edge === 'right' ) ||
( isRightToLeft && edge === 'left' );
const offset = isTrailingEdge ? 1 : 0;

// Update the currently known best candidate.
Expand Down