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

Allow user to choose how mouse position is computed in mouse events #62

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

Conversation

themoritz
Copy link
Contributor

Usage would be:

domEvent (Mousemove RelativeToOffset) el

Drawback is that it breaks usage of Mousemove, Mouseup and Mousedown.

An alternative would be to introduce event types like MousemoveOffset etc. But then it's not clear why Mousemove should yield (e.clientX, e.clientY)...

@ryantrinkle
Copy link
Member

I completely agree with the problem you've described here, but I worry about needing to make additional breaking changes in the future to support new use cases. For example, what about modifier keys? I'm not sure it's sustainable to try to foresee what the user might need.

Do you have any thoughts on how we can extend this to a more general interface?

@themoritz
Copy link
Contributor Author

I totally see your point. I guess if we want to solve the problem once and for all then all mouse related events should return an Event t MouseEvent, where MouseEvent gives me access to whatever aspect of the event I need. Let's assume this datatype is a record like in purescript-halogen. Then I could write the code like

e <- domEvent Mousemove el
let xyOffset = (mouseEventOffsetX &&& mouseEventOffsetY) <$> e

(Similarly, Keydown would yield an Event t KeyboardEvent with the corresponding accessors and so on...)

Ideally, we wouldn't need to recreate the MouseEvent datatype, but instead leverage on the functions that are already in ghcjs-dom by making them available to the user. Let's say we have a monadic fmap for events, then I could for example write

e <- domEvent Mousemove el -- e :: ev, IsMouseEvent ev
xOffset <- fmapM (liftIO . getOffsetX) e

But this is probably less user friendly than the first version

@ryantrinkle
Copy link
Member

Yep, I think that might be the way to go eventually! We can't do IO directly when mapping over Events, since that would make Reflex non-deterministic, but the user can use performEvent as necessary.

I think an approach like the purescript-halogen one might be best.

@themoritz themoritz force-pushed the mouseevent-position-type branch from 50dc715 to aa86b10 Compare June 7, 2016 15:17
@themoritz
Copy link
Contributor Author

The diff now shows how the halogen approach might look like. Next step would be to populate the records for the different event result types. Interestingly, for example uiEventScrollTop would not be a property of UIEvent, but we could still expose it in the UIEventResult record.

@themoritz
Copy link
Contributor Author

Okay, I added the properties that I think make sense. Some remarks:

  • I did not include properties that are deprecated, don't make sense in the context, or are not supported by ghcjs-dom. We can always add properties later though.
  • I removed scrollTop from UIEvent, because I think it is confusing as a property. Users could use wrapDomEvent to get it. An alternative would be to introduce a ScrollEventResult even if such an interface does not exist in JS, and to collect stuff like scrollTop in there.
  • FocusEvent has no useful properties, so I put () as EventResultType. maybe a data FocusEventResult = FocusEventResult would be better?

@hSloan hSloan added enhancement opinionated Requires judgement from senior developer or developer community follow up Follow up with Merge Requester to take some action labels Jan 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change enhancement follow up Follow up with Merge Requester to take some action opinionated Requires judgement from senior developer or developer community
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants