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

onDoubleClickNode handler never called when nodes are collapsible (onClickNode handler called twice instead) #294

Open
vsramanujan opened this issue Jan 19, 2020 · 3 comments

Comments

@vsramanujan
Copy link

vsramanujan commented Jan 19, 2020

Describe the bug
Only the click handler is called when nodes are collapsibe

To Reproduce
Steps to reproduce the behavior:

  1. Go to the live demo page
  2. Double click on node -> see the double click popup on the right
  3. Enable the collapsible property
  4. Double click on a node now -> only the single click handler is called twice and node collapses and comes back as well

Expected behavior
Node shouldn't collapse if its a double click and the double click handler should be called

Additional context
I'd be happy to raise a PR to fix this, but I do have a few questions that I'd like to get your thoughts on.

  1. Right now, double click is manually handled using a setTimeout hack in the onClickNode handler. Do we want this handling rewritten using the native ondblclick event?

  2. If we want to stick with how it is right now, then we could tweak onClickNode to accommodate this in a couple of ways.
    Right now, if the collapsible property is not set, the onClick handler is delayed by 300ms so that we can detect a doubleClick if it happens before then. But if the collapsible property is set, the click event handler is triggered immediately (albeit in the callback of setState for the collapse behaviour). Hence a double click with collapsible property on is handled as 2 single click events (which occurs after the collapse and reverting of collapse), while a double click without collapsible property on is treated as a doubleClick (and even if there is no doubleClick handler prop passed, the click handler prop is not called as well). So there is a discrepancy here.
    So, what do we do for the above discrepancy? Also, if there isn't any doubleClick handler prop that is passed, do we need to treat a double click as 2 single clicks and trigger the single click handler twice? (this is happening now when the collapsible property is set and not happening when it is not )

  3. To accommodate doubleClick during collapsible (if we don't rewrite with dblclick), we would need to delay the collapse handling code by 300ms as well whenever collapsible prop is passed so that we would invoke onDoubleClickNode instead of doing collapse twice when there is a doubleClick. Similar to question 2, we again have an option of doing this only when onDoubleClickNode is passed so that we don't always delay collapse by 300ms as well, and treat such a doubleClick as two single clicks . What do you think?

  4. For the behaviour to be uniform, we could just always setTimeout by 300ms (aka check for doubleClick) in onNodeClick and then handle things accordingly within it. Hence, a doubleClick would never trigger 2 node single click handlers and all 'click behaviours' will always happen after 300ms irrespective of what the props to the graph are.
    So, if its a doubleClick -> doubleClick handler (if present) will be called
    It its a singleClick and node collapsible -> collapse node and call single click handler
    It its a singleClick and node not collapsible -> just call single click handler

I personally prefer option 1 or 4, but am curious as to what you want to go with and what your views are on 2/3 to standardize behaviour.

@vsramanujan vsramanujan changed the title onDoubleClickNode handler never called when nodes are collapsible onDoubleClickNode handler never called (onClickNode handler called twice instead) when nodes are collapsible Jan 19, 2020
@vsramanujan vsramanujan changed the title onDoubleClickNode handler never called (onClickNode handler called twice instead) when nodes are collapsible onDoubleClickNode handler never called when nodes are collapsible (onClickNode handler called twice instead) Jan 19, 2020
@danielcaldas
Copy link
Owner

I would be very glad if you would attempt to re-implement this with native dblclick event.

FYI this has been attempted before: #202

image

But some time has passed since then, it might be worth to give a another shoot! PRs are most welcome!

If this does not work we can revaluate.

@mailuesp
Copy link

+1

@danielcaldas danielcaldas added the breaking change a breaking change in the library label Aug 14, 2020
@danielcaldas danielcaldas removed the breaking change a breaking change in the library label Sep 30, 2020
@danielcaldas
Copy link
Owner

Hey @vsramanujan if you (or anyone) are willing to give this a try, I think we better go with Option 3

To accommodate doubleClick during collapsible (if we don't rewrite with dblclick), we would need to delay the collapse handling code by 300ms as well whenever collapsible prop is passed so that we would invoke onDoubleClickNode instead of doing collapse twice when there is a doubleClick. Similar to question 2, we again have an option of doing this only when onDoubleClickNode is passed so that we don't always delay collapse by 300ms as well, and treat such a doubleClick as two single clicks . What do you think?

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

3 participants