-
Notifications
You must be signed in to change notification settings - Fork 64
refs #100
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
refs #100
Changes from 5 commits
088bbf7
90ea09b
796debd
aaa1ba8
00d51f6
dc2b66a
1b0d542
5395879
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -19,6 +19,7 @@ module React | |
, ReactRefs | ||
|
||
, Refs | ||
, Ref | ||
|
||
, Render | ||
, GetInitialState | ||
|
@@ -43,6 +44,8 @@ module React | |
|
||
, getProps | ||
, getRefs | ||
, readRef | ||
, writeRef | ||
, getChildren | ||
|
||
, readState | ||
|
@@ -65,7 +68,10 @@ module React | |
) where | ||
|
||
import Prelude | ||
|
||
import Control.Monad.Eff (kind Effect, Eff) | ||
import Data.Maybe (Maybe) | ||
import Data.Nullable (Nullable, toMaybe) | ||
import Unsafe.Coerce (unsafeCoerce) | ||
|
||
-- | Name of a tag. | ||
|
@@ -294,6 +300,26 @@ foreign import getRefs :: forall props state access eff. | |
ReactThis props state -> | ||
Eff (refs :: ReactRefs (read :: Read | access) | eff) Refs | ||
|
||
foreign import data Ref :: Type | ||
|
||
-- | Read named ref from Refs | ||
foreign import readRefImpl :: forall props state access eff. | ||
ReactThis props state -> | ||
String -> | ||
Eff (refs :: ReactRefs (read :: Read | access) | eff) (Nullable Ref) | ||
|
||
readRef :: forall props state access eff. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you please add comments to these as well? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
By the way: using zypher & webpack (with uglify plugin) on purescript-isomorphic-react-example, which is pulling DOM for very basic stuff (access to location, history, search the mounting point in the DOM), pulls ~2.3KB of data from There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In my opinion, I think |
||
ReactThis props state -> | ||
String -> | ||
Eff (refs :: ReactRefs (read :: Read | access) | eff) (Maybe Ref) | ||
readRef this name = toMaybe <$> readRefImpl this name | ||
|
||
foreign import writeRef :: forall props state access eff. | ||
ReactThis props state -> | ||
String -> | ||
Ref -> | ||
Eff (refs :: ReactRefs (write :: Write | access) | eff) Unit | ||
|
||
-- | Read the component children property. | ||
foreign import getChildren :: forall props state eff. | ||
ReactThis props state -> | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,6 +1,9 @@ | ||
module React.DOM.Props where | ||
|
||
import React (Event, EventHandlerContext, KeyboardEvent, MouseEvent, handle) | ||
import Control.Monad.Eff (Eff) | ||
import Control.Monad.Eff.Unsafe (unsafePerformEff) | ||
import Prelude (Unit, (<<<)) | ||
import React (Event, EventHandlerContext, KeyboardEvent, MouseEvent, ReactRefs, Ref, Write, handle) | ||
|
||
foreign import data Props :: Type | ||
|
||
|
@@ -297,6 +300,19 @@ radioGroup = unsafeMkProps "radioGroup" | |
readOnly :: Boolean -> Props | ||
readOnly = unsafeMkProps "readOnly" | ||
|
||
ref :: String -> Props | ||
ref = unsafeMkProps "ref" | ||
|
||
-- | You can use `writeRef` to store a reference on `Refs`. | ||
-- | ``` purescrript | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Quick note: should be |
||
-- | div [ refCb (writeRef this "inputEl") ] [...] | ||
-- | ``` | ||
refCb | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't like this name so much, what about something like There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I actually I don't like it either, |
||
:: forall access eff | ||
. (Ref -> Eff (refs :: ReactRefs (write :: Write | access) | eff) Unit) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just as a note, I think this may need to be (Nullable Ref -> Eff (refs :: ReactRefs (write :: Write | access) | eff) Unit) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From react docs: React will call the ref callback with the DOM element when the component mounts, and call it with null when it unmounts. And this is done to avoid memory leaks (DOM references used to leak in old IEs). If we write the ref only if it's not So if we change this type signature (which I do like, since things are explicitly typed then) then we also need to change There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed. I think it makes sense to write a There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks great! Quick question: Did we want this to be There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good point. Having here There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We will only need to add a comment, that if one is attaching the ref to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Great! Thanks! |
||
-> Props | ||
refCb cb = unsafeMkProps "ref" (unsafePerformEff <<< cb) | ||
|
||
rel :: String -> Props | ||
rel = unsafeMkProps "rel" | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you please add a comment here?