-
Notifications
You must be signed in to change notification settings - Fork 56
Added some window functions and unit tests #64
Conversation
|
Thanks for working on these. I'll come back to this later once |
|
|
|
@garyb Got the checks to pass |
garyb
left a comment
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.
Thanks for working on these! There's a few things I'd like removed or updated before merging though.
src/DOM/HTML/Window.purs
Outdated
|
|
||
| foreign import close :: forall eff. Window -> Eff (window :: WINDOW | eff) Unit | ||
|
|
||
| foreign import closed :: forall eff. Window -> Eff (window :: WINDOW | eff) Boolean |
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.
Please remove this, it's a nonstandard property - this library is intentionally only implementing standards, even when things are commonly available.
src/DOM/HTML/Window.purs
Outdated
|
|
||
| foreign import length :: forall eff. Window -> Eff (window :: WINDOW | eff) Int | ||
|
|
||
| foreign import minimize :: forall eff. Window -> Eff (window :: WINDOW | eff) Unit |
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.
Please remove this one too.
src/DOM/HTML/Window.purs
Outdated
| -> String | ||
| -> String | ||
| -> String | ||
| -> Eff (window :: WINDOW | eff) (Maybe Window) |
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.
You could implement this with Nullable instead and then use toMaybe. Something like this:
The FFI code can just be currying then, with no need to handle the Nothing / Just arguments.
src/DOM/HTML/Window.purs
Outdated
| -> (forall a. Maybe a) | ||
| -> Window | ||
| -> String | ||
| -> Eff (prompt :: PROMPT | eff) (Maybe String) |
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.
Another case where Nullable can be used.
|
@garyb Updated it How do you feel about extending purescript-nullable to have |
|
|
garyb
left a comment
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.
Sorry, just noticed another thing - the argument orders are all "javascripty" rather than "purescripty" - window should be the last argument in each case, as then you can do things like:
alert "hi" =<< window|
Rearranged it |
|
Thanks! |
No description provided.