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

Add doubleClick support to slot selection #575

Merged
merged 4 commits into from
Nov 7, 2017

Conversation

alxmiron
Copy link
Contributor

@alxmiron alxmiron commented Oct 4, 2017

This PR is a part of #563 issue

Purpose:

Selection object already has support for slot selection with action: 'click' property passed to onSelectSlot. We have to add support for double click interaction, with action: 'doubleClick' property passed.

Problem:

Now Selection object emits click event on every click, so if you click twice, it will emit 2 click events with a small delay.

Solution:

To emit doubleClick event inside Selection object, we have to check, wether first click is just a single click (then we emit click event), or it's the first click of double click (then we should mute it):

  1. User makes the first click. Don't emit click event yet. Create a timeout for 200ms (usual max delay between 2 clicks of double click). Remeber timestamp of the first click.
  2. User makes the second click. Check current time and the timestamp of first click. If it's less than 200ms, then emit doubleClick event and clear created timeout, else emit click event.
  3. If user haven't made the second click, emit click event inside created timeout

Disadvantage:

onSelectSlot handler for click events can be called only after 200ms after user click. I don't think we can make this delay less, because normal delay between 2 clicks of doubleClick event can be up to 170ms.

@alxmiron
Copy link
Contributor Author

@jquense Can you take a look at this PR, please

@jquense
Copy link
Owner

jquense commented Oct 17, 2017

hey there, thanks for the PR and your patience. I think the best option is to mimic browser doubleClick behavior, which is that the first click event happens, and if the another click happens within a configurable timespan (defualt 250ms) the second click is emitted as a doubleClick. So for every double click you get one click and one doubleClick event.

@alxmiron
Copy link
Contributor Author

alxmiron commented Oct 17, 2017

Actually, this is how I made this thing working, except two points:

  1. If we emit click event on the first click and doubleClick on the second one, onSelectSlot public method will be called twice (first time with action: 'click', and second - action: 'doubleClick'). Are you sure we haven't mute first click interaction?
  2. Should I change delay from 200ms (current value) to 250ms?

@jquense
Copy link
Owner

jquense commented Oct 17, 2017

sorry not sure if ytou are saying it already works like i suggested? Does the curretn code mute the first click? It shouldn't

@alxmiron
Copy link
Contributor Author

Yes, it mutes the first click. I described the reason in 1. of my previous comment

@jquense
Copy link
Owner

jquense commented Oct 17, 2017

Ok that's what I thought. Yes I'm saying we should change that behavior and instead emit the first click. THat's how doubleClick works natively on the web and we should copy that behavior

@alxmiron
Copy link
Contributor Author

Ok, you are right. I just checked it by myself

@alxmiron
Copy link
Contributor Author

@jquense Done. Now solution looks more simplified. When a user clicks an empty cell:

  • if difference with current time and time of last click event is less than 250ms - emit doubleClick event
  • else - emit click event

@alxmiron
Copy link
Contributor Author

@jquense please take a look at this

src/Selection.js Outdated

if (this._lastClickData && now - this._lastClickData.timestamp < clickInterval) {
// Double click event
this._lastClickData = {
Copy link
Owner

Choose a reason for hiding this comment

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

shouldn't this be cleared here? It seems like if you click 3 times really fast you'll get 1 click and 2 dblClick events instead of: click, dblClick, click

@alxmiron
Copy link
Contributor Author

alxmiron commented Nov 2, 2017

@jquense fixed

@alxmiron
Copy link
Contributor Author

alxmiron commented Nov 7, 2017

@jquense please take a look at this

src/Selection.js Outdated

if (
this._lastClickData &&
!this._lastClickData.isDblClick &&
Copy link
Owner

Choose a reason for hiding this comment

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

why add this isDblClick instead of setting _lastClickData to null after a double click

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't see much difference between checking for a flag and checking for null value. _lastClickData = null seems obscure for me

Copy link
Owner

@jquense jquense Nov 7, 2017

Choose a reason for hiding this comment

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

I hear you, though the opposite strikes me as more obscure, at the moment we are tracking some metadata that isn't used which isn't clear. I think i'd be clearer to clear out the last click data if there is no last click data :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed. Clear _lastClickData on double click event

@jquense jquense merged commit fd91e81 into jquense:master Nov 7, 2017
@jquense
Copy link
Owner

jquense commented Nov 7, 2017

thanks a lot!

@alxmiron
Copy link
Contributor Author

alxmiron commented Nov 7, 2017

Finally! 🎊 Thanks

@alxmiron alxmiron deleted the feature/563-3 branch November 7, 2017 15:40
Copy link

@abhibansal60 abhibansal60 left a comment

Choose a reason for hiding this comment

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

is this feature still functional?

i want to use doubleClick on onSelectSlot? how can i do that

@cutterbl
Copy link
Collaborator

If you 'showCode' on the onDoubleClickEvent prop documentation you'll get an example of how you can handle this.

@dr-skot
Copy link

dr-skot commented Mar 27, 2023

Actually for this behavior you need the onSelectSlot prop. Check for action === 'doubleClick' in slotInfo parameter.

@dr-skot
Copy link

dr-skot commented Mar 27, 2023

It looks like there's no access to the MouseEvent, or its target. If double-clicking on a slot adds a new calendar event, it would be nice to anchor a popup form to the slot. But I don't see how to get the slot element. Is there a way?

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.

5 participants