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

Request: HOC to prevent drag from starting on given element #47

Closed
Jessidhia opened this issue Sep 5, 2016 · 10 comments
Closed

Request: HOC to prevent drag from starting on given element #47

Jessidhia opened this issue Sep 5, 2016 · 10 comments

Comments

@Jessidhia
Copy link

The use case would be a non-draggable button nested inside a draggable list item.

Right now, the workarounds are to either attach native click events to the button (requiring a ref and extra lifecycle handlers), or to add a delay and hope the users click the button faster than the delay (but that reduces drag responsiveness). Otherwise, the dragging event takes precedence and blocks clicks to any clickable thing inside the SortableElement.

SortableHandle could also be a workaround, but that comes at the expense of being able to drag the SortableElement by dragging its background.

@burtyish
Copy link

burtyish commented Sep 5, 2016

I ran into this problem too. I have a list of draggable items with inputs inside. Using the inputs is impossible since a click on an input is caught by SortableElement.
I've fallen back to using a drag handle in each item.

@Jessidhia
Copy link
Author

The best workaround I've found so far is to directly attach DOM handlers on
the interactable elements for both mousedown and touchstart, and just
calling stopPropagation on the event (definitely don't return false).

This prevents the event from bubbling to the container's handler that then
prevents the default.
On Mon, 5 Sep 2016 at 20:15 Yishai Burt [email protected] wrote:

I ran into this problem too. I have a list of draggable items with inputs
inside. Using the inputs is impossible since a click on an input is caught
by SortableElement.
I've fallen back to using a drag handle in each item.


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#47 (comment),
or mute the thread
https://github.com/notifications/unsubscribe-auth/AAEdfQsLTddqSbRGLSypvB8FdlJXb9r_ks5qm_nKgaJpZM4J0sIY
.

@zaygraveyard
Copy link
Contributor

I think the best solution would be to add a shouldSortStart prop on SortableContainer.
That gets called before starting to drag and gets passed the event, the drag starts only if it returns true.
Which could render the handle useless.

I can work on a PR if needed.

@clauderic
Copy link
Owner

clauderic commented Sep 5, 2016

There are indeed a number of ways to approach this, between a cancel function as described by @zaygraveyard, another HOC, or even a data-attribute. At present time, the only way to work around this is to use the method described by @Kovensky

I'm still debating which would be the most practical, though I think the HOC approach would become cumbersome if you have multiple different elements that need to be excluded.

Would love to hear people's thoughts on the cancel function approach / if you can think of other ways to solve this.

clauderic pushed a commit that referenced this issue Sep 5, 2016
… Prevent right click from causing sort start (#46)
@clauderic
Copy link
Owner

clauderic commented Sep 5, 2016

Just published 0.0.8 to npm with support for shouldCancelStart, which should solve most use cases.

Default implementation:

function shouldCancelStart(e) {
    // Cancel sorting if the event target is an `input`, `textarea`, `select` or `option`
    if (['input', 'textarea', 'select', 'option'].indexOf(e.target.tagName.toLowerCase()) !== -1) {
        return true; // Return true to cancel sorting
    }
}

@Jessidhia
Copy link
Author

Nice, will try to use this.

I can't use the default implementation as-is, though, as my interactable element is a button.

@clauderic
Copy link
Owner

Should be pretty straightforward, let me know if you run into any issues.

@Jessidhia
Copy link
Author

@clauderic after trying to actually adapt this to work for my use case, it does seem like a pass-through wrapping component would be best...

That the element inside the SortableElement instance is non-draggable is a property of the item, not of the SortableContainer instance; it's just that the Container should have a way to deal with it. Right now, this requires writing a contrived check inside the Container.

A pass-through wrapper (something used like <SortableClickable><Children/></SortableClickable>) would allow for easily indicating which portion of the tree should be clickable. It seems to me that SortableHandle is almost like this, but instead of rendering React.Children.only(this.props.children) it requires a function to tell it which component to render.

Supporting a button (like in my use case) also gets complicated when you actually have elements inside the button. This is the implementation I would end up having to use:

// same as the default whitelist in 0.0.8
// eslint-disable-next-line react/sort-comp
static whitelist = new Set(['input', 'textarea', 'select', 'option'])
handleCancelStart (e) {
  // default behavior
  if (ClassName.whitelist.has(e.target.tagName.toLowerCase())) {
    return true
  }
  for (let target = e.target; target !== this.contentDiv; target = target.parentElement) {
    if (target.tagName.toLowerCase() === 'button') {
      return true
    }
  }
}

This requires saving an additional ref to the div that contains the SortableContainer instance (could be done with withRef, but requiring one to begin with is the problem) to know where to stop looking. shouldCancelStart also runs afoul of react/jsx-handler-names.

PS: A lot of the props given to a SortableContainer instance look like they should be given to the SortableContainer constructor instead (like helperClass, lockAxis, useDragHandle; having to specify them where the instance is used is leaking internals, unless the resulting component takes in children instead of rendering the SortableElement instance itself like in the examples).

@clauderic
Copy link
Owner

clauderic commented Sep 6, 2016

@Kovensky I'm not discarding the idea of a HOC to achieve this. As I mentioned, shouldCancelStart is a starting point which should cover most use cases.

As for naming conventions, since this is not an event handler per-se, I don't see anything wrong with it. A function name should convey meaning, and I found shouldCancelStart to be a lot more meaningful than handleStart.

Supporting a button (like in my use case) also gets complicated when you actually have elements inside the button.

Agreed, a HOC would certainly hide this complexity, which is why it's not out of the cards. That said, looking at the code, it doesn't look all that complicated; that's fairly similar to how the SortableHandle is implemented, so ultimately it's just a matter of hiding complexity.

PS: A lot of the props given to a SortableContainer instance look like they should be given to the SortableContainer constructor instead (like helperClass, lockAxis, useDragHandle; having to specify them where the instance is used is leaking internals

I disagree. All of those props are specific to each instance. Say you want to render two SortableContainers on the page, one using the x axis and one using the y axis, you'd need to have two different enhanced SortableContainer constructors. This also gives you the ability to change these properties on the fly.

@KrzysztofMaly
Copy link

In my case the solution appeared to be much simpler than all that.
I just used onMouseDown event instead of onClick.

DimitarNestorov pushed a commit to codemotionapps/react-sortable-hoc that referenced this issue Feb 4, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

5 participants