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

Connecting monitor.getClientOffset to a DropTarget does not refresh props #179

Closed
grassick opened this issue Jun 3, 2015 · 22 comments
Closed

Comments

@grassick
Copy link

@grassick grassick commented Jun 3, 2015

I created a DropTarget and connected getClientOffset as follows:

targetCollect = function (connect, monitor) {
  return {
    connectDropTarget: connect.dropTarget(),
    isOver: monitor.isOver(),
    clientOffset: monitor.getClientOffset()
  }
}

However, when moving drag within the component, the target does not receive updated props with each wiggle of the mouse (only on enter and exit). The target spec's hover is called however repeatedly. Is there a reason that the clientOffset prop is not injected each time it is changed?

@gaearon
Copy link
Member

@gaearon gaearon commented Jun 3, 2015

Oh, good point. That's right, I didn't anticipate this usage. Currently, the only way to opt into tracking client offset is to use DragLayer. Otherwise, for performance reasons, it only updates the props if something concerning the drop target itself has changed.

I'd say it's an API problem, and there may be several solutions:

  • Forbid reaching into getClientOffset() and similar methods from inside collect function and tell to use DragLayer instead (easy but dumb);
  • Automatically figure out that user wants to track the offset and subscribe to the offset changes if this is the case (harder but more consistent).

I think I'd accept a PR doing the second option. You can check DragLayer implementation to see how it subscribes to the offset changes.

@gaearon gaearon added the bug label Jun 3, 2015
@grassick
Copy link
Author

@grassick grassick commented Jun 3, 2015

I looked into the second option, but it's tricky as collect is a function, so it's not possible/easy to find out which monitor functions might be called.

My use case is a dragging around of widgets which are snapped into location, possibly displacing other widgets in a relatively complex way (think dragging program icons on an Android). It's really the target that has to rearrange itself depending on the exact hover location, while the drag itself is perfectly ok as an HTML5 snapshot. I can do it with the target spec's hover updating the state of the target component. So I'll leave this for now and work around it. Thanks for an amazing library; I have something decent working in a very few hours of work.

@grassick grassick closed this Jun 3, 2015
@gaearon
Copy link
Member

@gaearon gaearon commented Jun 3, 2015

Let's keep this open for now. It's the same as #172: I didn't anticipate people using deltas inside canDrop or the collecting function. I'm not going to fix this right away, but may after I free up in a month or so.

@ibash
Copy link

@ibash ibash commented Feb 27, 2016

I also ran into this issue. What are your thoughts on letting clients pass a flag into DropTarget spec to subscribe to offset changes? It's one more "gotcha" for the user, but is simpler than manually subscribing/unsubscribing to offsets.

@jnatherley
Copy link

@jnatherley jnatherley commented Mar 2, 2016

Could you not perhaps use RxJS-DOM to observe the dragover event on the element and update the state with mouse coordinates? Hacky solution perhaps.

  componentDidMount() {
    this._events.dragOver = DOM.fromEvent(findDOMNode(this.refs.grid), 'dragover')
                            .throttle(200)
                            .subscribe(this.getMouseCoords)
  }
@ibash
Copy link

@ibash ibash commented Mar 2, 2016

I ended up grabbing the monitor and subscribing to offset changes like the DragLayer. I also ended up tapping into the internal registry in places. I don't think what I want to do is too unusual, so IMO it could be addressed by simple api changes.

@jchristman
Copy link

@jchristman jchristman commented Apr 26, 2016

Has anything come of this? I would like to monitor the client offset in my DropTarget rather than in the custom drag layer because I feel like it's easier to change the DropTarget's structure when I can access the hover location from the DropTarget instead of the custom drag layer.

@bado22
Copy link

@bado22 bado22 commented Apr 27, 2016

+1 I'd like to do exactly what @jchristman is trying to do.

@noah79
Copy link

@noah79 noah79 commented May 4, 2016

+1. Running into this and wasting a lot of time trying to understand why getClientOffset() works fine in canDrop() - which does get called repeatedly... - but no way to forward to my control 'where' over the item we are. I need to be able to insert above and below a treenode, not just "on" it. canDrop() has no way that I can find that lets me actually tell my component which of the many "types" of drops the node has so that my control can redisplay accordingly. Changing the prop in canDrop resulted in an error. Subscribing to onMouseMove doesn't work during drag and drop operations. This has been a huge waste of time so far...any help would be appreciated.

@ibash, would you mind posting a gist of what you did to solve your problem?

@noah79
Copy link

@noah79 noah79 commented May 4, 2016

Switching to using hover finally did it for me.

` hover: (props: MyProps, monitor: DropTargetMonitor, component: ReportItemRow) => {
if (component != null) {
const clientOffset = monitor.getClientOffset();

        // Is the dragging node dragged above/below us as opposed to "on" us?

        const elementRect = document.elementFromPoint(clientOffset.x, clientOffset.y).getBoundingClientRect();

        const percentageY = (clientOffset.y - elementRect.top) / elementRect.height;

        // Divide the box up into .25 .5 .25
        const insertDragAndDropMagnetPercent = .25;

        if (insertDragAndDropMagnetPercent >= percentageY) {
            component.setState({ dropTarget: "above" });
        }
        else if (1 - insertDragAndDropMagnetPercent >= percentageY) {
            component.setState({ dropTarget: "inside" });
        }
        else {
            component.setState({ dropTarget: "below" });
        }
    }
},`
@jchristman
Copy link

@jchristman jchristman commented May 4, 2016

@noah79, in the meantime of waiting for something to come of this, you can solve your problem by creating a number of Drop Targets and putting them under your treenodes. Somthing like this:

         _____
         _____| <-- drop div1 (above treenode1/below top of list)
treenode1_____| <-- drop div2 (on treenode1)
         _____| <-- drop div3 (below treenode1/above treenode2)
treenode2_____| <-- drop div4 (on treenode 2)
         _____| <-- drop div5 (below treenode2/above bottom of list)

You end up with 2n+1 drop targets, where n is the number of elements in your list. Then, based on which of the divs is being hovered over, you can change how the tree list looks. I've done something very similar to this to get around not being able to access getClientOffset() for now, and I did not notice a hit on performance. The height of each drop div should be 1/2 the height of the treenode1 line and the first drop div should be located 1/4 of the height of the treenode1 line higher than the first line. That is, your CSS should look like this:

.dropdiv-container {
    position: absolute;
    top: -0.25em; /* If the top of this container is aligned with the top of the treenode list initially */
}
.dropdiv {
    height: 0.5em; /* If the height of treenode1 line is 1em with no padding */
}
<div className='dropdiv-container'>
    { renderDropTargets(2 * listLength + 1) }
</div>

Does that make sense?

@ibash
Copy link

@ibash ibash commented May 4, 2016

Here's a gist of how I solved this. The trick is to just reach into the
react-dnd internals and use the global monitor directly. Take a look the
DragDropMonitor.js source to get an idea of what methods are available.
ref: https://github.com/gaearon/dnd-core/blob/master/src/DragDropMonitor.js

You'll have to excuse the coffeescript :)

  # in the component being dragged, get access to the dragDropManager by adding
  # it to the contextTypes
  #
  # dragDropManager is an instance of this:
  # https://github.com/gaearon/dnd-core/blob/master/src/DragDropManager.js

  @contextTypes: {
    dragDropManager: React.PropTypes.object
  }

  # because we can receive events after the component is unmounted, we need to
  # keep track of whether the component is mounted manually.
  #
  # @_monitor is what lets us access all the nice internal state - it is an instance of this:
  # https://github.com/gaearon/dnd-core/blob/master/src/DragDropMonitor.js

  componentWillMount: () =>
    @_isMounted = true
    @_monitor = @context.dragDropManager.getMonitor()

    @_unsubscribeToStateChange = @_monitor.subscribeToStateChange(@_onStateChange)
    @_unsubscribeToOffsetChange = @_monitor.subscribeToOffsetChange(@_onOffsetChange)

  componentWillUnmount: () =>
    @_isMounted = false
    @_monitor = null

    @_unsubscribeToStateChange()
    @_unsubscribeToOffsetChange()

  # we can access dragging / dropping state by accessing the monitor 

  _onStateChange: () =>
    return unless @_isMounted

    # When we stop dragging reset the counter state and hide all cursors.
    if @_previousIsDragging && !@_monitor.isDragging()
      console.log('no longer dragging')
    @_previousIsDragging = @_monitor.isDragging()

  _onOffsetChange: () =>
    return unless @_isMounted

    # mouse is the x/y coordinates
    mouse = @_monitor.getClientOffset()

    # item is the drag item
    item = @_monitor.getItem()

    # if you want to check if a dragged item is over a target, you need the
    # targetId -- in the DropTarget wrapper you can pass it in like:
    #
    #   (connect, monitor) ->
    #     {
    #       targetId: monitor.targetId,
    #       connectDropTarget: connect.dropTarget()
    #     }
    #
    # and then you can use it like below

    @_monitor.isOverTarget(@props.targetId))

Does this make sense? If not I can add more details

@KnutHelland
Copy link

@KnutHelland KnutHelland commented Jul 29, 2016

I was also running into this today and wasted some hours. What about leaving a note about it in the documentation under DropTarget -> The Collecting Function? That would at least spare some others for frustrations.

@KnutHelland
Copy link

@KnutHelland KnutHelland commented Jul 29, 2016

Btw. one other really ugly hack around this issue can be to send in the coordinates as state from dropTarget.hover():

const dropTarget = {
    hover(props, monitor, component) {
        // HACK! Since there is an open bug in react-dnd, making it impossible
        // to get the current client offset through the collect function as the
        // user moves the mouse, we do this awful hack and set the state (!!)
        // on the component from here outside the component.
        component.setState({
            currentDragOffset: monitor.getClientOffset(),
        });
    },
    drop() { /* ... */ },
};

Of course there are tons of more correct ways to do this that would avoid abusing of setState like that. But at least this little hack is very condensed and may do the job until this issue is eventually fixed. It lets you hack around the bug without changing other components and/or files and without relying on library internals.

Edit: this is basically the same as noah79 does, I just didn't read his code before now.

@mhuggins
Copy link

@mhuggins mhuggins commented Sep 23, 2016

Is there really no way to get the mouse position on drop? My target component doesn't need to know about it, so setState isn't an option for me. I just need the coordinates in order to trigger a redux action.

@mhuggins
Copy link

@mhuggins mhuggins commented Sep 23, 2016

Please ignore my last message. I see now that drop and endDrag are two different pieces of functionality, and getting cursor positioning within drop works as expected. :)

@serle
Copy link

@serle serle commented Nov 23, 2016

From my tests, react-dnd is consuming the mouse move events as soon as an item is being dragged over the target. If it were possible for the html-backend not to consume this event then the target component could just place a normal mousemove listener on its dom conditionally when the isOver property is set to true. Thereafter this listener could pick up the mouse position (in the normal way) when something is being dragged over it. I got this to work temporarily using setState from the drag() method react is giving warnings about setting state in the middle of a render transition.

@calrsom
Copy link

@calrsom calrsom commented Apr 6, 2017

This tripped me up too. +1 for an API solution or at least something on the FAQ.

@jedwards1211
Copy link
Contributor

@jedwards1211 jedwards1211 commented Apr 10, 2017

Is there any plan to support this?
I have an idea for a hack-free workaround: make a wrapper for DropTarget that also wraps hover to trigger another call to the collecting function and a re-render if the collecting function returns new values.

@augbog
Copy link

@augbog augbog commented Aug 1, 2017

+1 for this issue. use case I have is I have several drop targets that are in close proximity of each other. It seems weird that when hovering, the proper drop target highlights but actually dropping will drop on a lower target depending on what direction you hovered from.

@tambeta
Copy link

@tambeta tambeta commented Nov 22, 2018

I stumbled on this issue researching why offsets aren't updating, seems like it still needs a workaround.
I took up @jedwards1211's idea and here's what I came up with. The HOC has mostly the same interface as vanilla DropTarget, but accepts collectRapidly in its options which is a list of props to query via the collecting function + pass down on every hover event. Passing all props indiscriminately from the collecting function caused some weirdness and we cannot pass connect to the collector at all, so there is a guard against querying connectDropTarget "rapidly".

So instead of DropTarget(types, target, collect) you would use RapidFireDropTarget(types, target, collect, {collectRapidly: ['offset']}), where offset is presumably something receiving its value from the monitor.getOffset family of functions.

import React from 'react';
import {DropTarget} from 'react-dnd';
import pick from 'lodash/pick';

function RapidFireDropTarget(types, spec, collect, options={}) {
  let collectProps = {};
  const prevHover = spec.hover;
  const {collectRapidly = []} = options;

  const dummyConnect = {
    dropTarget: () => () => {
      throw new Error('Rapidly collected props cannot include results from `connect`');
    }
  };

  const WrappedComponent = Component => {
    return class extends React.Component {
      render() {
        return <Component {...this.props} {...collectProps} />;
      }
    };
  };

  spec.hover = (props, monitor, component) => {
    const allCollectProps = collect(dummyConnect, monitor);

    collectProps = pick(allCollectProps, collectRapidly);
    component.forceUpdate();

    if (prevHover instanceof Function) {
      prevHover(props, monitor, component);
    }
  }

  return (Component) => {
    return DropTarget(types, spec, collect, options)(WrappedComponent(Component));
  };
}

export default RapidFireDropTarget;
@stale
Copy link

@stale stale bot commented Jul 6, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the wontfix label Jul 6, 2019
@stale stale bot closed this Jul 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet