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 support for createRef API #172

Merged
merged 11 commits into from
Aug 10, 2019
Prev Previous commit
Next Next commit
Tidy up
  • Loading branch information
elliotdavies committed Jul 1, 2019
commit a469d235b41653b4781630cb11bcf5bf7c544922
3 changes: 1 addition & 2 deletions src/React.purs
Original file line number Diff line number Diff line change
@@ -67,14 +67,13 @@ module React

import Prelude

-- import Data.Nullable (Nullable)
import Effect (Effect)
import Effect.Exception (Error)
import Effect.Uncurried (EffectFn1)
import Prim.Row as Row
import React.Ref as Ref
import Type.Row (type (+))
import Unsafe.Coerce (unsafeCoerce)
import React.Ref as Ref

-- | Name of a tag.
type TagName = String
3 changes: 1 addition & 2 deletions src/React/DOM/Props.purs
Original file line number Diff line number Diff line change
@@ -2,8 +2,6 @@ module React.DOM.Props where

import Prelude

import Web.HTML.HTMLElement (HTMLElement)
-- import Data.Nullable (Nullable)
import Effect (Effect)
import Effect.Uncurried (mkEffectFn1)
import React.Ref as Ref
@@ -21,6 +19,7 @@ import React.SyntheticEvent
, SyntheticUIEvent
, SyntheticWheelEvent
)
import Web.HTML.HTMLElement (HTMLElement)
Copy link
Contributor

@ethul ethul Jul 25, 2019

Choose a reason for hiding this comment

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

Just a quick note. This is coming from purescript-web-html, but I didn't notice the dependency in the bower file. Is it being pulled in from another dependency?

On this note, I am unsure if we want this as a dependency since runtimes like react native, etc., would not necessarily require it. So far we have been attempting to keep dependencies at a minimum. That being said, I am not completely opposed to adding the dependency, but wanted to open it up for discussion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ethul Well spotted; I think I forgot to push the change to the bower file. I had added "purescript-web-html": "^2.2.0" locally.

I suppose that we could have a local HTMLElement type here and not use the library version. It would save on the dependency, but on the other hand anyone using DOM refs would have to write a conversion function (i.e. unsafeCoerce), which might be annoying.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for adding the dependency.


foreign import data Props :: Type