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

Drag and drop: allow dragging to the beginning and end of a document #56070

Merged
merged 6 commits into from
Nov 17, 2023
Merged

Drag and drop: allow dragging to the beginning and end of a document #56070

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
e144126
Try passing in a ref to allow dragging to the top and bottom of the e…
andrewserong Nov 13, 2023
11376a9
Update naming
andrewserong Nov 14, 2023
7635836
Implement in the site editor
andrewserong Nov 15, 2023
1fe9f39
Fix gap between post title and top of iframe
andrewserong Nov 15, 2023
f93ac8e
Add __unstable prefix in useInnerBlocksProps
andrewserong Nov 16, 2023
c6d84ac
Fix typo in comment
andrewserong Nov 17, 2023
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
8 changes: 6 additions & 2 deletions packages/block-editor/src/components/inner-blocks/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -169,8 +169,11 @@ const ForwardedInnerBlocks = forwardRef( ( props, ref ) => {
* @see https://github.com/WordPress/gutenberg/blob/HEAD/packages/block-editor/src/components/inner-blocks/README.md
*/
export function useInnerBlocksProps( props = {}, options = {} ) {
const { __unstableDisableLayoutClassNames, __unstableDisableDropZone } =
options;
const {
__unstableDisableLayoutClassNames,
__unstableDisableDropZone,
__unstableDropZoneElement,
Copy link
Contributor

Choose a reason for hiding this comment

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

If we want to make this private, prefixing is not the way anymore. I think we're now avoiding any new __unstable or __experimental prefix.

Copy link
Contributor

Choose a reason for hiding this comment

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

I just read your message above, you should use the lock/unlock API to mark this private. These days prefixing or not doesn't impact our policies for new APIs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestion and extra context @youknowriad! I've had a go at doing this over in #56308, would love your feedback to see if that's what you had in mind. Due to how useInnerBlocksProps is written and used, the solution wound up being a little indirect.

A potential alternative would be for us to stabilise dropZoneElement for useInnerBlocksProps and BlockList if we're comfortable with it as an API. We could then still look into further follow-ups that abstract it away a little further for the post and site editors (or folks building custom block editors).

Copy link
Contributor

Choose a reason for hiding this comment

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

If we're making this private with the intention of removing it later and making the drop zone part of BlockCanvas as suggested above, my main question as to whether we should go one way or the other is if it would be useful at all to be able to set a drop zone in inner blocks. #26049 has been open for 3 years with no comments and few links to it, and the issue itself only links to one potential use case, so there doesn't seem to be a huge demand for such an API.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

#26049 has been open for 3 years with no comments and few links to it, and the issue itself only links to one potential use case, so there doesn't seem to be a huge demand for such an API.

I suppose the presence of that issue nudges me slightly toward "let's go for private for now" as it means we can then continue to explore the idea of allowing it for (likely 3rd party) container blocks in follow-up PRs (by digging into the potential use case). If we find it isn't useful, we can abandon it and go with making it part of BlockCanvas, and if it is useful, we can make the API public (and also potentially still explore making it an internal implementation detail of BlockCanvas). In both cases, it sounds like the private approach in #56308 could make for a decent interim place for now? I agree it doesn't sound like there's much demand for it for core container blocks to set their own element 🤔

I don't feel strongly about which way we go, though, my main interest is getting us to an interim state we feel comfortable with for now 🙂

Copy link
Contributor

Choose a reason for hiding this comment

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

#26049 has been open for 3 years with no comments and few links to it, and the issue itself only links to one potential use case, so there doesn't seem to be a huge demand for such an API.

A lot of folks complain generally about the drag and drop experience being clunky without really providing specifics or knowing the underlying reason. I don't think I did a good job of writing that issue, I should really have led with the problem rather than a solution, and I think that has meant not many comments.

and the issue itself only links to one potential use case

There are a few blocks it happens with - Cover, Media & Text and Group. It happens any time there's padding around the inner blocks element.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It happens any time there's padding around the inner blocks element.

Ah, excellent. Thanks for confirming, Dan! That gives us something concrete to investigate here 👍

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 can reproduce with the Cover block. I've added a screengrab to #26049 (comment) to demonstrate the problem. I'm happy to pick up that task next as it's very related to this PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Another update: given that solutions to #26049 will likely involve the dropZoneElement prop on useInnerBlocksProps, I've opened up another PR to stabilise the dropZoneElement prop, over in #56313, as an alternative to making it private. We can likely go either way, but stabilising is the simpler code change.

} = options;
const {
clientId,
layout = null,
Expand Down Expand Up @@ -211,6 +214,7 @@ export function useInnerBlocksProps( props = {}, options = {} ) {
);

const blockDropZoneRef = useBlockDropZone( {
dropZoneElement: __unstableDropZoneElement,
rootClientId: clientId,
} );

Expand Down
5 changes: 4 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 @@ -127,7 +127,8 @@ export function getDropTargetPosition(

/**
* @typedef {Object} WPBlockDropZoneConfig
* @property {string} rootClientId The root client id for the block list.
* @property {?HTMLElement} dropZoneElement Optional element to be used as the drop zone.
* @property {string} rootClientId The root client id for the block list.
*/

/**
Expand All @@ -136,6 +137,7 @@ export function getDropTargetPosition(
* @param {WPBlockDropZoneConfig} dropZoneConfig configuration data for the drop zone.
*/
export default function useBlockDropZone( {
dropZoneElement,
// An undefined value represents a top-level block. Default to an empty
// string for this so that `targetRootClientId` can be easily compared to
// values returned by the `getRootBlockClientId` selector, which also uses
Expand Down Expand Up @@ -235,6 +237,7 @@ export default function useBlockDropZone( {
);

return useDropZone( {
dropZoneElement,
isDisabled,
onDrop: onBlockDrop,
onDragOver( event ) {
Expand Down
7 changes: 7 additions & 0 deletions packages/edit-post/src/components/visual-editor/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -411,6 +411,13 @@ export default function VisualEditor( { styles } ) {
: `${ blockListLayoutClass } wp-block-post-content` // Ensure root level blocks receive default/flow blockGap styling rules.
}
layout={ blockListLayout }
__unstableDropZoneElement={
// When iframed, pass in the html element of the iframe to
// ensure the drop zone extends to the edges of the iframe.
isToBeIframed
? ref.current?.parentNode
: ref.current
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting, I'd have expected this to be contentRef but I guess it works because contentRef includes ref? I'm assuming that what matters is a shared ref between BlockCanvas and dropZoneElement, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep! Yeah, interestingly enough it's contentRef in the site editor because that's the name of the direct ref, whereas in the post editor contentRef is the merged ref. So, ref contains the .current reference to the node. From a little digging (I think this is how it works) in the post editor contentRef is what gets passed to BlockCanvas as it contains a callback function that will update the list of refs to be merged, but we need to use ref.current to grab a reference to the element itself. If we attempt to access contentRef directly (or pass it anywhere else), we just get the callback function rather than the element, as that's what's returned by useMergeRefs.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oooh I see, thanks!

/>
</RecursionProvider>
</BlockCanvas>
Expand Down
6 changes: 6 additions & 0 deletions packages/edit-site/src/components/block-editor/site-editor-canvas.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -134,6 +134,12 @@ export default function SiteEditorCanvas() {
isTemplateTypeNavigation,
}
) }
__unstableDropZoneElement={
// Pass in the html element of the iframe to ensure that
// the drop zone extends to the very edges of the iframe,
// even if the template is shorter than the viewport.
contentRef.current?.parentNode
}
layout={ LAYOUT }
renderAppender={ showBlockAppender }
/>
Expand Down
Loading