-
Notifications
You must be signed in to change notification settings - Fork 4.2k
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 drag and drop indicator above first block and RTL drop indicators #33024
Fix drag and drop indicator above first block and RTL drop indicators #33024
Conversation
Size Change: +43 B (0%) Total Size: 1.05 MB
ℹ️ View Unchanged
|
'wp-block', | ||
className | ||
) } | ||
className={ classnames( 'block-list-appender', className ) } |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 😓
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
It might be too late for 5.8 for this one, but it'd be nice if it was possible to include it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested and worked pretty good!
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, | ||
}; |
There was a problem hiding this comment.
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 };
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works as described 🚢
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work!
At some point it might be good to have some e2e testing for the indicator. Cc @ockham because you were looking for places that need for e2e test coverage.
// Only show the in-between inserter between blocks, so when there's a | ||
// previous and a next element. | ||
const showInsertionPointInserter = | ||
previousElement && nextElement && isInserterShown; |
There was a problem hiding this comment.
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.
Just referencing #28185 if we want to add some e2e tests for the indicator :) |
Thanks for the reviews, will look into some follow-ups to improve the insertion point code and e2e tests. |
…#33024) * Update logic * RTL and block appender fixes * Fix RTL positioning calculations * Avoid adding wp-block to the appender
…#33024) * Update logic * RTL and block appender fixes * Fix RTL positioning calculations * Avoid adding wp-block to the appender
Description
The drag and drop indicator before the first block hasn't been working (I think since the insertion point component was used for the drop indicator).
This PR fixes that. While working on it and testing my changes, I noticed a few issues with drag and drop for RTL languages, so I've fixed those too.
How has this been tested?
Dropping to the ends of block lists still requires some precision (the cursor must be within the block list element), so be aware of that when testing.
LTR (vertical block list)
LTR (horizontal block list)
RTL (vertical block list)
Prerequisite, Install the RTL Tester plugin, switch to RTL, and change this function to return
true
- https://github.com/WordPress/gutenberg/blob/trunk/packages/i18n/src/create-i18n.js#L400.RTL (horizontal block list)
Prerequisite, Install the RTL Tester plugin, switch to RTL, and change this function to return
true
- https://github.com/WordPress/gutenberg/blob/trunk/packages/i18n/src/create-i18n.js#L400.Also worth testing the in-between inserter and the indicator when hovering a block in the main block library, which all use the same InsertionPoint component.
Screenshots
Types of changes
Bug fix (non-breaking change which fixes an issue)
Checklist:
*.native.js
files for terms that need renaming or removal).