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 onSomethingDown callbacks #910

Closed
wants to merge 1 commit into from

Conversation

ericluap
Copy link
Contributor

@ericluap ericluap commented Jun 11, 2020

Adds callbacks for the different clicks on mouse down.

This is useful for certain things like changing the position of the cursor in an Input component on mouse down.

Copy link
Member

@glennsl glennsl left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @SirFurness! I'm not sure this is really needed on Clickable though. onMouseDown is already available on View, which is what you'd typically use to implement something like this. I could see the value of having onRightMouseDown for convenience if that was a really common need, but I don't think it is. Consequently this mostly just complicates the API I think. I'm certainly open for arguments to the contrary though.

Comment on lines +23 to +29
~onDown: unit => unit=?,
~onRightClick: unit => unit=?,
~onRightDown: unit => unit=?,
~onDoubleClick: unit => unit=?,
~onDoubleDown: unit => unit=?,
~onAnyClick: NodeEvents.mouseButtonEventParams => unit=?,
~onAnyDown: NodeEvents.mouseButtonEventParams => unit=?,
Copy link
Member

Choose a reason for hiding this comment

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

This should be onMouseDown, onRightMouseDown etc. to disambiguate it from for example onKeyDown.

@ericluap
Copy link
Contributor Author

You're probably right that it's not a common need. I was mainly thinking in terms of future improvements to the Input component. It needs the cursor to change on left click mouse down and it will eventually need a word to be highlighted on double click mouse down.

I don't think any component should use onMouseDown from a View because it is called for any type of mouse button. This leads to things like the Slider component moving with a right-click drag. My current thinking is just that anything that cares about mouse clicks should use a Clickable so that things like onMouseDown work only with left clicks and code checking for what mouse button was used does not need to be duplicated. It could also make the intent more clear (e.g. that Slider is meant to be clicked).

I guess it depends on how the Clickable component is meant to be used. Maybe there should be a different component?

@glennsl
Copy link
Member

glennsl commented Jun 12, 2020

I'm honestly not really sure what Clickable is supposed to be either, as the events it exposes could easily have been made available directly on View.

I don't think any component should use onMouseDown from a View because it is called for any type of mouse button. This leads to things like the Slider component moving with a right-click drag.

The mouse button is passed in the events args. That's how you're able to implement this. And this is analogous to how onMouseDown works on the Web. For something as rarely needed as this I personally think that's fine.

Left-only click, right click and double click is common enough that I think it warrants separate events, and they fit well with Clickable. But the rest I'm not so sure about design-wise. It would be nice to have a holistic idea and some overall guiding principles for where we're going with this I think.

@ericluap
Copy link
Contributor Author

Sounds good! A document with some guiding design principles would be quite nice, especially with regards to big things like #489.

@ericluap ericluap closed this Jun 13, 2020
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