Add DOM.HTML.History.#75
Conversation
|
I think |
|
Oh good idea. I'll do the same for |
|
Can you make Travis happy? It's just lint/style stuff, the fixes can probably be applied automatically. |
|
Thanks @garyb. I don't know how I missed those warnings. I probably did |
garyb
left a comment
garyb
left a comment
There was a problem hiding this comment.
Sorry, I noticed a few other things. This'll be the end of the requests, honest!
| foreign import go :: forall e. History -> Delta -> Eff (history :: HISTORY | e) Unit | ||
| foreign import pushState :: forall e. History -> Foreign -> DocumentTitle -> URL -> Eff (history :: HISTORY | e) Unit | ||
| foreign import replaceState :: forall e. History -> Foreign -> DocumentTitle -> URL -> Eff (history :: HISTORY | e) Unit | ||
| foreign import state :: forall e. History -> Eff (history :: HISTORY | e) Foreign |
There was a problem hiding this comment.
Sorry I didn't notice this earlier, but can we put the History argument at the end for all of these? That's more consistent with the rest of the API, and is done so you can write things like:
go -2 =<< history =<< windowThere was a problem hiding this comment.
I followed the pattern in Location module, but I see those functions only have one arg, but I look closer and see you're right. I'll update.
| -- DocumentTitle will set value of `document.title` | ||
| newtype DocumentTitle = DocumentTitle String | ||
| newtype Delta = Delta Int | ||
| newtype URL = URL String -- Unsure how to better type this. |
There was a problem hiding this comment.
Can you derive Eq, Ord, and Newtype instances for these please?
I know what you mean about URL, but I think it's fine as a newtype here, as the API we're providing with this library is supposed to be dumb anyway.
| @@ -0,0 +1,47 @@ | |||
| // module DOM.HTML.History | |||
There was a problem hiding this comment.
We can drop these comments now, the compiler switched to using paths to figure out associated FFI files a few versions ago.
There was a problem hiding this comment.
Ah neat, I didn't know that - I thought it was purely convention.
|
Awesome suggestions. I've updated and pushed a new commit. |
garyb
left a comment
garyb
left a comment
There was a problem hiding this comment.
Everything looks great now apart from one thing... need to update the FFI implementations to move history to the last argument too! 😄
|
Ah, yeah, good catch. I am using this branch in my project but I didn't update to this latest commit yet. I should do that to provide some assurance it's working as it should. I'll post here after I've done that. It'll be 1-3 days, I guess. |
|
Thanks! |
|
I haven't finished testing this in my app, but that's fine b/c I can just make another MR if I need to fix something. Thanks @garyb for reviewing! |
2 participants
The type signature of
statemight be wrong:state :: forall a e. History -> Eff (history :: HISTORY | e) a. I'm not using it, but after some brief exploring of that property I believe it could be any shape, so I puta. The browser puts one type of data in there, but users could put any type there viapushState, per the history spec.Fixes #74