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

Refactor Draggable to decouple drag handle from the DOM node being dragged #9311

Merged
merged 24 commits into from Sep 3, 2018
Merged
Changes from 1 commit
Commits
Show all changes
24 commits
Select commit Hold shift + click to select a range
b435722
Add withDraggable HOC
nosolosw Aug 24, 2018
b5a35cd
Deprecate Draggable
nosolosw Aug 24, 2018
18ef675
Convert block-draggable into class
nosolosw Aug 27, 2018
2d48f1a
Inline Draggable component
nosolosw Aug 27, 2018
26a0b6f
Use withDraggable
nosolosw Aug 27, 2018
2ca2e4a
Pass props to wrapped component
nosolosw Aug 29, 2018
eb0c491
withDraggable is responsible to call the props passed to the wrapped …
nosolosw Aug 29, 2018
f545653
BlockDraggable: adapt to new API
nosolosw Aug 29, 2018
7610b9e
Fix dragend event handler
nosolosw Aug 29, 2018
c9b44f2
withDraggable's event handlers take control
nosolosw Aug 29, 2018
07d1749
Make setTimeout property available within withDraggable
nosolosw Aug 29, 2018
06ae3ba
Do not pass dragover to children
nosolosw Aug 29, 2018
d9a1807
Make Draggable ready to process children functions
nosolosw Aug 30, 2018
25297ce
Remove any traces of withDraggable HOC
nosolosw Aug 30, 2018
cc43d33
Move deprecation to correct version
nosolosw Aug 30, 2018
0427c19
BlockDraggable: convert it back to a functional component
nosolosw Aug 30, 2018
f340eca
Update docs
nosolosw Aug 30, 2018
7829554
Update docs
nosolosw Aug 30, 2018
2d4c207
Clarify BlockDraggable abstraction
nosolosw Aug 30, 2018
96c337c
Update docs
nosolosw Aug 30, 2018
7cd7f62
Update deprecation
nosolosw Aug 30, 2018
f568bf5
Add note in CHANGELOG
nosolosw Aug 30, 2018
755e0aa
Update version that will depracate the behavior
nosolosw Aug 30, 2018
32006d3
Add deprecation within 4.0 section
nosolosw Sep 3, 2018
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.
+25 −29
Diff settings

Always

Just for now

BlockDraggable: convert it back to a functional component

  • Loading branch information...
nosolosw committed Aug 30, 2018
commit 0427c192c5d54e60affc2205bfa0d06ac513ca2f
@@ -6,37 +6,33 @@ import classnames from 'classnames';
/**
* WordPress dependencies
*/
import { Component } from '@wordpress/element';
import { Draggable } from '@wordpress/components';

class BlockDraggable extends Component {
render() {
const { isDragging, elementId, transferData, onDragStart, onDragEnd } = this.props;
const className = classnames( 'editor-block-list__block-draggable', {
'is-visible': isDragging,
} );
const BlockDraggable = ( { isDragging, elementId, transferData, onDragStart, onDragEnd } ) => {
const className = classnames( 'editor-block-list__block-draggable', {
'is-visible': isDragging,
} );

return (
<Draggable
elementId={ elementId }
transferData={ transferData }
onDragStart={ onDragStart }
onDragEnd={ onDragEnd }
>
{
( { onDraggableStart, onDraggableEnd } ) => (
<div
className={ className }
onDragStart={ onDraggableStart }
onDragEnd={ onDraggableEnd }
draggable
>
<div className="editor-block-list__block-draggable-inner"></div>
</div> )
}
</Draggable>
);
}
}
return (
<Draggable
elementId={ elementId }
transferData={ transferData }
onDragStart={ onDragStart }
onDragEnd={ onDragEnd }
>
{
( { onDraggableStart, onDraggableEnd } ) => (

This comment has been minimized.

Copy link
@youknowriad

youknowriad Aug 30, 2018

Contributor

Minor: Maybe we should also pass the draggable prop here. That way we can just do { ...props } in the inner div. The idea is: The Draggable component provides all the necessary props to make dragging work.

This comment has been minimized.

Copy link
@youknowriad

youknowriad Aug 30, 2018

Contributor

I guess my comment assumes we rename onDraggableStart and onDraggableEnd into onDragStart and onDragEnd

This comment has been minimized.

Copy link
@nosolosw

nosolosw Aug 30, 2018

Author Member

That way the Draggable's onDragStart/onDragEnd properties would shadow the BlockDraggable's onDragStart / onDragEnd props. I know we can avoid that by using the ...props trick but it feels more obscure to me, like a smell of something that's not right.

I like the current proposal better because it has a clearer "API contract": Draggable provides event handlers for you to connect in a specific DOM node. That's it.

This comment has been minimized.

Copy link
@youknowriad

youknowriad Aug 30, 2018

Contributor

Fair enough. Noting that it's a common pattern though in the React world (see https://github.com/paypal/downshift)

<div
className={ className }
onDragStart={ onDraggableStart }
onDragEnd={ onDraggableEnd }
draggable
>
<div className="editor-block-list__block-draggable-inner"></div>
</div> )
}
</Draggable>
);
};

export default BlockDraggable;
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.