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

Hooks Ref'ing Updates #1280

Merged
merged 32 commits into from Mar 21, 2019
Merged

Hooks Ref'ing Updates #1280

merged 32 commits into from Mar 21, 2019

Conversation

darthtrevino
Copy link
Member

@darthtrevino darthtrevino commented Mar 20, 2019

Key Updates to the Hooks API per our recent discussion in #1148

  • Remove ref key on dragSource, dropTarget spec.
  • useDrag and useDrop return connector functions, which can now be chained and use as refs
  • Change the top-level useDrag and useDrop functions to accept a function and memoize keys so that the spec objects aren't created on every render.

Simple Case

let [, drag] = useDrag({/*...*/})
return <div ref={drag}>...</div>

Chained Case

let [, drag] = useDrag({/*...*/})
let [, drop] = useDrop({/*...*/})
return <div ref={node => drag(drop(node))}>...</div>

or

let ref = useRef(null)
let [, drag] = useDrag({/*...*/})
let [, drop] = useDrop({/*...*/})
drag(drop(ref))
return <div ref={ref}>...</div>

Drag Previews

Remove preview key on dragSource spec, use connector function instead
Remove return ref from useDragPreview
Rename useDragPreview to useDetachedComponent
Add a DragPreviewImage component to render drag preview images on the output tree. (Note: The useDragPreview mechanism was removed inherit limitations in the HTML5 drag-and-drop API (https://developer.mozilla.org/en-US/docs/Web/API/DataTransfer/setDragImage) limit us to images)

let [, drag, preview] = useDrag(() => ({/*...*/}))
return (
    <div ref={drag}>
        <DragPreviewImage connect={preview} src="imageSrc.png" /> 
        ...
    </div>
)

Known Issues

  • the shallowEquals check in useCollector is necessary for performance, but right now the hooks don't render correctly at all with it, and I'm not sure why.
  • the draglayer example is wonky, dragged item is not disappearing and custom drag layer is wrong size
  • the sortable card example is wonky, dragged item gets out of sync with view
  • the stress test example does not function due to issue above.

@darthtrevino darthtrevino changed the title Work on removing the ref-aspects of hooks. (WIP) Hooks Updates (WIP) Mar 20, 2019
@darthtrevino
Copy link
Member Author

I'm not really a fan of using a callback to memoize. I'd like to run some tests to see if a function arg really makes a difference here.

…ose since the ref objects will be harmless on FCs
Because of the limitations with drag previews, we can't use arbitrary components, only Image elements. Because of this, we've retooled the DisconnectedDragPreview HOC to be just a DragPreviewImage component which can render disconnected images.
This mechanism for showing drag previews is much, much simpler, but still allows clients to use image previews as components.
@darthtrevino darthtrevino changed the title Hooks Updates (WIP) Hooks Ref'ing Updates Mar 21, 2019
@darthtrevino darthtrevino merged commit 1d7207e into master Mar 21, 2019
@darthtrevino darthtrevino deleted the feature/hooks_refs_removal branch March 21, 2019 19:20
@@ -103,10 +102,8 @@ export function DragDropContext(
public render() {
return (
<Provider value={childContext}>
<Decorated
{...this.props}
ref={isClassComponent(Decorated) ? this.ref : undefined}
Copy link

@yched yched Mar 26, 2019

Choose a reason for hiding this comment

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

The removal of the "isClassComponent(Decorated)" logic causes a "Function components cannot be given refs" error in 7.4.0 when DragDropContext is used to wrap a non-class component, which was supported so far.

(I'm guessing this also affects other parts of the API where the isClassComponent() check was removed)

Copy link
Member Author

Choose a reason for hiding this comment

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

I realized that and added the check back in

Copy link
Member Author

Choose a reason for hiding this comment

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

Ahh, I see, I only restored the check in the decorateHandler. I'll restore those checks in a patch version today.

Copy link

Choose a reason for hiding this comment

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

Awesome, thanks !

darthtrevino added a commit that referenced this pull request Feb 3, 2022
* docs: clean up native file example

* test: add an example case to demonstrate flaw with current hook api

* fix: replace refs in useDrag,useDrop with connector functions

* feat: work on hooks, perf improvements

* refactor: clean up usedrag/usedrop hooks

* refactor: clean up the number of hook files

* feat: use callback refs in hooks api

* feat: memoize drag/drop specs

* fix: class api typings

* feat: remove the 'preview' option of the drag spec

* feat: update previews, documentation

* refactor: move isRef

* refactor: create SourceConnector, TargetConneector classes

* fix: hooks isuses

* refactor: make the hooks field constant in the connector classes

* fix: reconnect connectors on target change

* docs: clean up knight component

* fix: add typings to useCollector arguments

* fix: tighten typings on dragdrop monitors

* fix: some liveness issues, hooks are still wonky atm

* refactor: decouple useCollector/useMonitor output from connector types

* docs: update usedetachedcomponent docs

* fix: correct issues with detached previews, rename the hook to useDetachedPreview

* docs: remove previewproperty from dragspec in useDetachedPreview

* revert: eliminate implicit memoization. BYOM(emoization of specs)

* refactor: clarify the ref mechanism is decorateHandler. Remove recompose since the ref objects will be harmless on FCs

* fix: scope down the drag preview wrapper

Because of the limitations with drag previews, we can't use arbitrary components, only Image elements. Because of this, we've retooled the DisconnectedDragPreview HOC to be just a DragPreviewImage component which can render disconnected images.

* docs: add drag-preview-image docs to sidebar

* refactor: replace the portaling mechanism with a renderless component

This mechanism for showing drag previews is much, much simpler, but still allows clients to use image previews as components.

* docs: add some notes to the new examples
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants