Draggable: Allow elementId based elements to be attached to the ownerDocument body #49911
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Size Change: +26 B (0%) Total Size: 1.37 MB
ℹ️ View Unchanged
|
andrewserong
added
[Feature] UI Components
andrewserong
requested review from
mirka,
ciampo,
apeatling,
ramonjd and
tellthemachines
ramonjd
approved these changes
Apr 19, 2023
mirka
requested changes
Apr 19, 2023
mirka
reviewed
Apr 19, 2023
| @@ -8,9 +8,16 @@ Note that the drag handle needs to declare the `draggable="true"` property and b | |||
|
|
|||
| The component accepts the following props: | |||
|
|
|||
| ### `attachToElementWrapper`: `boolean` | |||
There was a problem hiding this comment.
Would it be more intuitive to invert this prop to be attachToOwnerDocument? I find it cleaner to avoid true-by-default props whenever possible.
There was a problem hiding this comment.
Also append rather than attach might be a more standard way to name it.
| @@ -8,9 +8,16 @@ Note that the drag handle needs to declare the `draggable="true"` property and b | |||
|
|
|||
| The component accepts the following props: | |||
|
|
|||
| ### `attachToElementWrapper`: `boolean` | |||
|
|
|||
| Whether to attach the cloned element to the element's wrapper when using `elementId`. Setting to false will attach the cloned element to the `ownerDocument` body. | |||
There was a problem hiding this comment.
I was a bit thrown off by the "when using elementId" part because elementId is supposed to be a required prop.
Suggested change
| Whether to attach the cloned element to the element's wrapper when using `elementId`. Setting to false will attach the cloned element to the `ownerDocument` body. | |
| Whether to attach the cloned element to the element's wrapper. Setting to false will attach the cloned element to the `ownerDocument` body. |
There was a problem hiding this comment.
I've reworded it, but retained reference to the id, as otherwise it could be confusing that appendToOwnerDocument defaults to false but is still in use when someone's using the experimental drag component. Happy to keep tweaking / feel free to go with different wording if it still feels off!
| @@ -38,7 +39,7 @@ const DefaultTemplate: ComponentStory< typeof Draggable > = ( args ) => { | |||
| <div> | |||
| <p>Is Dragging? { isDragging ? 'Yes' : 'No!' }</p> | |||
| <div | |||
| /* eslint-disable no-restricted-syntax, eslint-comments/disable-enable-pair */ | |||
| /* eslint-disable-next-line no-restricted-syntax */ | |||
| id="draggable-example-box" | |||
There was a problem hiding this comment.
Duplicate ids are not going to work in Docs view (?path=/docs/components-draggable--default) where all stories are rendered in the same document.
| @@ -81,3 +82,69 @@ export const Default: ComponentStory< typeof Draggable > = DefaultTemplate.bind( | |||
| {} | |||
| ); | |||
| Default.args = {}; | |||
|
|
|||
| const AttachElementIdTemplate: ComponentStory< typeof Draggable > = ( | |||
There was a problem hiding this comment.
I don't quite think this is worth the code duplication of adding a separate template. For example, you can add a story description in a code comment like this, which eliminates the need for a description rendered inside the story itself:
gutenberg/packages/components/src/base-control/stories/index.tsx
Lines 52 to 59 in 85e2d4e
Comment on lines
1
to
7
| .draggable-storybook-wrapper-element .draggable-storybook-element { | ||
| background-color: #99faa4; | ||
| } | ||
|
|
||
| .draggable-storybook-element { | ||
| background-color: #fa99c8; | ||
| } |
There was a problem hiding this comment.
I wonder if it'd be better to just have a single attachToOwnerDocument=true story that demonstrates the z-index use case (instead of this CSS scoping demo), which is the exact use case that prompted you to implement this prop. Especially because with this CSS stuff, it's kind of impossible to understand what's going on without looking at the source code.
There was a problem hiding this comment.
That's a good question — I added the CSS example mostly to demonstrate that the original behaviour was retained (the true by default example). However, now that you've suggested going with a new prop that is false by default, there's less of a need for that.
I'll have a play around with z-index and see if I can consolidate on a single example that better reflects my use case. Thanks!
|
Thanks for all the detailed feedback @mirka, much appreciated! Lots of good suggestions. I've done the following:
The result is that we should now be able to play around with this in Storybook with just the Default story and the Append To Owner Document story. Here's a quick couple of gifs to demonstrate:
Let me know if you'd like to see any other changes! |
mirka
approved these changes
Apr 20, 2023
Co-authored-by: Lena Morita <lena@jaguchi.com>
|
Flaky tests detected in 4a90cff. 🔍 Workflow run URL: https://github.com/WordPress/gutenberg/actions/runs/4759406586
|
andrewserong
deleted the
update/draggable-to-optionally-attach-elementid-element-to-body
branch
|
Nice job @mirka on the review! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What?
Update the
Draggablecomponent to accept anappendToOwnerDocumentprop that allows consumers to toggle whereelementIdbased elements are attached. It defaults tofalse, and allows setting totrueto make the element be attached to theownerDocumentbody instead.Why?
There are use cases for using an
elementIdbased element to clone, but where the consumer needs the cloned element to be attached to the owner document body instead of the element's wrapper (that is, use similar behaviour as exists currently when a custom drag component is used). I encountered this while working on #49820 where I need the cloned element to be attached to the body instead, so that I can get the draggable chip to render above the drop indicator line.Why not just use a custom drag component instead? In the case of the list view, we can avoid unnecessarily rendering additional components (e.g. a custom drag chip) by re-using the block list items that are already rendered. Also, the
elementIdlogic is very useful, as it already ensures that the cloned element is in the same position as the original element.However, there are likely lots of cases where styling rules expect the cloned element to be attached to the wrapper. For this reason, I've added the prop, but set it to
falseby default.How?
appendToOwnerDocumentprop, that defaults tofalse.ownerDocument.bodyinstead of the element wrapper (this borrows from the existing behaviour when an__experimentalDragComponentis used.Testing Instructions
Run storybook locally, and then navigate to the Draggable component:
Quick local links to the two added stories:
Screenshots or screencast
To demonstrate the difference, I've updated the storybook example so that the wrapper of the draggable element has a
z-indexset that is lower than thez-indexof the paragraph that flags whether or not the component is being dragged. WhenappendToOwnerDocumentis set totrue, then the cloned draggable element does not receive the z-index of the element wrapper, so the cloned element can go above the paragraph.appendToOwnerDocumentset tofalseappendToOwnerDocumentset totrue