purescript-contrib / purescript-react

React Bindings for PureScript
MIT License
400 stars 65 forks source link

Refactor events #144

Closed ethul closed 6 years ago

ethul commented 6 years ago

@natefaubion - Changes we discussed earlier in https://github.com/purescript-contrib/purescript-react/pull/140#issuecomment-381353147

ethul commented 6 years ago

We can definitely pull those out of the row. It would be a bit nicer to not have to propagate the eff through.

On Sun, Apr 15, 2018 at 12:45 Nathan Faubion notifications@github.com wrote:

@natefaubion commented on this pull request.

In src/React/SyntheticEvent.purs https://github.com/purescript-contrib/purescript-react/pull/144#discussion_r181589267 :

= ( bubbles :: Boolean , cancelable :: Boolean , currentTarget :: NativeEventTarget , defaultPrevented :: Boolean , eventPhase :: Number , isTrusted :: Boolean , nativeEvent :: NativeEvent

  • , preventDefault :: Eff eff Unit
  • , isDefaultPrevented :: Eff eff Boolean
  • , stopPropagation :: Eff eff Unit
  • , isPropagationStopped :: Eff eff Boolean

Is it necessary to add these as rows in the event? All SyntheticEvents support these methods, and if they weren't in the row then we wouldn't need to propagate the eff variable.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/purescript-contrib/purescript-react/pull/144#pullrequestreview-112244056, or mute the thread https://github.com/notifications/unsubscribe-auth/AAVYy7Zit1lbLTG8IaH7iaR7E07XUT97ks5to3khgaJpZM4TVdpf .

ethul commented 6 years ago

I've removed the eff functions from SyntheticEvent. Thanks for taking a look at this.

Does this seem good to go? If so, I will make a release a new release of the library. Unless there is anything else we should add in to the breaking changes.

ethul commented 6 years ago

@natefaubion Do you think this is good to go? Also, any further thoughts on adding-in the label for Eff?

natefaubion commented 6 years ago

I think it's fine to omit the label for now.

ethul commented 6 years ago

Sounds good. Thanks.