Implementing window.history #35
Conversation
… Fixed return value type for history.Length
There was a problem hiding this comment.
This looks very reasonable to me. I left 1 style suggestion, but I don't have any other comments.
Only thing... I think I remember discussing this with @dominikh and the tricky part was figuring out what type to use for state. Using interface{} seems okay, but maybe @dominikh had something elaborate in mind? If not, I think it's a fine starting point, isn't it?
dom.go
Outdated
| } | ||
|
|
||
| func (h *history) Back() { | ||
| h.Object.Call("forward") |
There was a problem hiding this comment.
Style suggestion. For consistency with above h.Get and the rest of the package, you can skip skip .Object since it's an embedded struct, just do h.Call("forward").
Here, and in 4 places below.
|
The issue is that data put into the history will be serialized by the browser. In turn, when you get it back out, you won't get an object of the Go type you put in. That makes the API rather awkward to use. |
|
@dominikh I understand there might be an issue here with the state field. I know currently, this value is not required by the browser. AFAIK it's used as an optional object. Instead of an interface{}, do you think it might be better suited to utilize a map[string]string or even a map[string]interface{}? |
|
@shurcooL - I made some changes based on your request. Please let me know if I missed anything |
|
Sorry to follow up @dominikh - just wanted to see if you had any suggestions. I want you to be satisfied with the implementation. At the same time, I really need window.History on my current project. :D |
Would it make sense to use |
dom.go
Outdated
| } | ||
|
|
||
| func (h *history) Length() int { | ||
| return h.Get("state").Int() |
There was a problem hiding this comment.
Did you mean to write h.Get("length").Int() here?
|
I'm deferring this PR, and all decisions, to @shurcooL. |
|
@shurcooL - Reported issue was fixed, I also made an additional fix of similar nature to "back" |
|
Following up here. @itsmontoya, have you seen my question in #35 (comment)? |
Resolves #41.