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.

Sign up for GitHub

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

Jump to bottom

Fix double click behavior is different from native #1551

Open
wants to merge 3 commits into
base: develop
Choose a base branch
from
Open

Fix double click behavior is different from native #1551

wants to merge 3 commits into from

Conversation

sasensi
Copy link
Contributor

@sasensi sasensi commented Oct 8, 2018

Description

Changes based on @DSnopov suggestion in #1451.

  • time delta is calculated between 2 down events (instead of between up and down)
  • a check is made to only emit double click if distance between 2 down points is under 3 pixels

Related issues

Checklist

  • Code conforms with the JSHint rules (npm run jshint passes)

@sasensi sasensi mentioned this pull request Oct 8, 2018
Open
@sasensi sasensi requested a review from lehni October 8, 2018 14:26
lehni
lehni reviewed Oct 9, 2018
View reviewed changes
src/view/View.js
// difference:
dblClick = hitItem === clickItem
&& (Date.now() - clickTime < 300);
downItem = clickItem = hitItem;
Copy link
Member

Choose a reason for hiding this comment

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

@sasensi this removes the check for clicks on the same item entirely... Are you sure we want this? Is it fine to do so because double-clicks are only considered within 3 pixels distance? But what if the first click was on a different item?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, you're right, I hadn't noticed that. But even if I don't manage to make it fail practically, from a theoretical point of view, it would be better to also check that item is the same for both clicks.
In new version, I also ensure that triple clicking an item don't produce 2 double click events.

@sasensi sasensi force-pushed the fix/double-click-behavior-is-different-from-native branch from 47de271 to b16813c Compare October 10, 2018 07:30
@lehni
Copy link
Member

lehni commented Oct 20, 2018

@sasensi this looks good now! Could you add unit tests for it also, now that we have the infrastructure for them in place?

@sasensi sasensi force-pushed the fix/double-click-behavior-is-different-from-native branch from b16813c to d5420a0 Compare November 3, 2018 12:26
@sasensi
Copy link
Contributor Author

sasensi commented Nov 3, 2018

@lehni I added unit test, ignore failing build as it is only in node v11 (see explanation here).

@lehni
Copy link
Member

lehni commented Jul 12, 2019

@sasensi could you rebase this on top of develop, so the tests pass?

@lehni lehni force-pushed the develop branch 2 times, most recently from 3cc7fa3 to a9ebe47 Compare December 14, 2019 19:29
@lehni lehni force-pushed the develop branch 2 times, most recently from 39b63a1 to b90a711 Compare June 19, 2020 17:45
@lehni lehni force-pushed the develop branch 3 times, most recently from 4a6bf6f to 0f4afc5 Compare June 22, 2020 13:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lehni lehni lehni left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

The doubleclick event should behave like the native DOM dblclick event
2 participants
@sasensi @lehni