Refactor events #144
Refactor events #144
Conversation
src/React/SyntheticEvent.purs
Outdated
| , preventDefault :: Eff eff Unit | ||
| , isDefaultPrevented :: Eff eff Boolean | ||
| , stopPropagation :: Eff eff Unit | ||
| , isPropagationStopped :: Eff eff Boolean |
There was a problem hiding this comment.
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.
|
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 ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In src/React/SyntheticEvent.purs
<#144 (comment)>
:
> = ( 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
<#144 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAVYy7Zit1lbLTG8IaH7iaR7E07XUT97ks5to3khgaJpZM4TVdpf>
.
|
|
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. |
src/React/SyntheticEvent.purs
Outdated
| type SyntheticEvent = Record (SyntheticEvent' ()) | ||
| import Data.Symbol (class IsSymbol, SProxy(..), reflectSymbol) | ||
|
|
||
| type SyntheticEvent r |
There was a problem hiding this comment.
Should these rows be closed? For example, it's not possible to have both a mouse event and an animation event.
There was a problem hiding this comment.
Yes. These can be closed. Good catch.
src/React/SyntheticEvent.purs
Outdated
| animationName :: forall eff r. SyntheticEvent_ (SyntheticAnimationEvent' r) -> Eff eff String | ||
| animationName = get (SProxy :: SProxy "animationName") | ||
|
|
||
| clipboardData :: forall eff r. SyntheticEvent_ (SyntheticClipboardEvent' r) -> Eff eff NativeDataTransfer |
There was a problem hiding this comment.
Additionally, I think that accessors could be polymorphic.
clipboardData :: forall eff r. SyntheticEvent_ (clipboardData :: NativeDataTransfer | r) -> Eff eff NativeDataTransfer
| => SProxy l | ||
| -> SyntheticEvent_ s | ||
| -> Eff eff a | ||
| get l r = unsafeGet (reflectSymbol l) r |
There was a problem hiding this comment.
This carries a fully polymorphic effect variable. Should it have a label?
There was a problem hiding this comment.
Are you suggesting we added in something Eff (sythenticEvent :: SYNTHETIC_EVENT) a? That works for me.
There was a problem hiding this comment.
Possibly. Seems like overkill to have another label for react stuff, but I'm not sure what the solution is short of consolidating the other labels we have. With 0.12 Effect transition coming up I wonder if it even matters.
There was a problem hiding this comment.
I am leaning toward not adding an effect label, unless we decide it is necessary. Agreed that with the 0.12 changes, it may not be worthwhile adding here. But I am open to ideas.
|
@natefaubion Do you think this is good to go? Also, any further thoughts on adding-in the label for |
|
I think it's fine to omit the label for now. |
|
Sounds good. Thanks. |
@natefaubion - Changes we discussed earlier in #140 (comment)