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 event doubleClick handler #567

Merged
merged 2 commits into from
Oct 3, 2017
Merged

Add event doubleClick handler #567

merged 2 commits into from
Oct 3, 2017

Conversation

alxmiron
Copy link
Contributor

@alxmiron alxmiron commented Oct 1, 2017

This PR is part of #563 issue

New:

  • onDoubleClickEvent handler
    Callback fired when a calendar event is clicked twice.
    (event: Object, e: SyntheticEvent) => any

All changes are similar to onSelectEvent handler

src/Calendar.js Outdated
@@ -112,6 +112,24 @@ class Calendar extends React.Component {
onSelectSlot: PropTypes.func,

/**
* Callback fired when empty calendar cell was clicked twice.
Copy link
Owner

Choose a reason for hiding this comment

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

"when an empty calendar [...]"

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

src/Calendar.js Outdated
* (date: Date, e: SyntheticEvent) => any
* ```
*/
onDoubleClickAllDaySlot: PropTypes.func,
Copy link
Owner

Choose a reason for hiding this comment

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

I don't think we have a normal click equivalent to this one?

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 decided to separate onDoubleClickSlot and onDoubleClickAllDaySlot. Because both of them can get same date parameter, but handling behavior is usually different for clicks on regular cells and all-day cells.
What do you think?

@@ -28,6 +28,7 @@
height: auto;
line-height: normal;
white-space: nowrap;
pointer-events: all;
Copy link
Owner

Choose a reason for hiding this comment

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

?

Copy link
Contributor Author

@alxmiron alxmiron Oct 2, 2017

Choose a reason for hiding this comment

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

This point is important.
As .rbc-row-content is overlaying .rbc-day-bg (in month view), that is listening to doubleClick events, we have to add pointer-events: none to .rbc-row-content, and pointer-events: all to all active elements inside it (events, popup link, day number link) read more

Copy link
Owner

Choose a reason for hiding this comment

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

I think that may be a problem, first it won't work in older browsers and second I think it is going to break the drag and drop code as well, since they also mess with pointer-events.

Maybe we can narrow the scope of this PR a bit, and only add doubleClick handlers where there is already a click handler, that will make sure we have a nice parity and we can talk about adding new handlers in a separate PR/issue?

Copy link
Contributor Author

@alxmiron alxmiron Oct 2, 2017

Choose a reason for hiding this comment

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

No, I checked. It doesn't make problems with DnD. Moreover, pointer-events properties are already used in some places of DnD addon. I can't see any other solutions, how to catch doubleClick events under .rbc-row-content overlay (remember, that clicked date should be calculated and passed to handler. this is a limitation)

Ok, I'll leave onDoubleClickEvent in this PR, and onDoubleClickSlot and onDoubleClickAllDaySlot will be moved to another one, where we can discuss the problem above

@alxmiron alxmiron changed the title #563: Add doubleClick handlers Add event doubleClick handler Oct 3, 2017
@alxmiron
Copy link
Contributor Author

alxmiron commented Oct 3, 2017

PR description was changed
Empty slot handlers were moved to another PR #570
onDoubleClickEvent is very similar to onSelectEvent, so this PR can be merged

src/Calendar.js Outdated
* Callback fired when a calendar event is clicked twice.
*
* ```js
* (event: Object, e: SyntheticEvent) => any
Copy link
Owner

Choose a reason for hiding this comment

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

should be => void for the return type i believe

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jquense done

Copy link
Owner

@jquense jquense left a comment

Choose a reason for hiding this comment

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

just that one nit

@jquense jquense merged commit 6c517dd into jquense:master Oct 3, 2017
@alxmiron alxmiron deleted the feature/563 branch October 3, 2017 16:18
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.

2 participants