Skip to content

Conversation

@dmitshur
Copy link
Member

Due to the way browsers work, if the user manually edits the URL to change the hash to another snippet ID and presses enter, the first time it will cause a hash change event (typically scrolls page to the matching anchor, without reloading page). Second time the user presses enter, since the hash has not changed, it will instead reload the page, and in our case reload the snippet.

Therefore, before this PR, if the user manually edits the URL in browser to another snippet ID, first time it did not actually load it. After this PR, it will always try to load the snippet whose ID is in the URL box.

I think this leads to a better user experience as it eliminates a possible confusing behavior. It's also more efficient than reloading entire page, since that is not necessary.


Code-wise, I realize the XHR request code is starting to get out of hand. I tried to factor out the common code into a loadSnippet func or so, but it didn't quite work because of minor differences and need for local variable access.

I think a better way to go would be to add helpers to XHR package to make binary XHR requests more easily, since it's a pretty common requirement. Then, all XHR request code can go from:

req:=xhr.NewRequest("GET", "http://"+snippetStoreHost+"/p/"+id) req.ResponseType=xhr.ArrayBuffergofunc(){err:=req.Send(nil) iferr!=nil||req.Status!=200{scope.Apply(func(){scope.Set("output", []Line{Line{"type": "err", "content": `failed to load snippet "`+id+`"`}}) }) return } data:=js.Global.Get("Uint8Array").New(req.Response).Interface().([]byte) scope.Apply(func(){scope.Set("code", string(data)) }) }()

To simply:

data, err:=xhr.SendBinary("GET", "http://"+snippetStoreHost+"/p/"+id, nil) iferr!=nil||req.Status!=200{scope.Apply(func(){scope.Set("output", []Line{Line{"type": "err", "content": `failed to load snippet "`+id+`"`}}) }) return } scope.Apply(func(){scope.Set("code", string(data)) })

Actually, that's not that much shorter. :/ But still cleaner I think.

Due to the way browsers work, if the user manually edits the URL to change the hash to another snippet ID and presses enter, the first time it will cause a hash change event (typically scrolls page to the matching anchor, without reloading page). Second time the user presses enter, since the hash has not changed, it will instead reload the page, and in our case reload the snippet. Therefore, before this PR, if the user manually edits the URL in browser to another snippet ID, first time it did not actually load it. After this PR, it will always try to load the snippet whose ID is in the URL box. I think this leads to a better user experience as it eliminates a possible confusing behavior. It's also more efficient than reloading entire page, since that is not necessary.
@dmitshur
Copy link
MemberAuthor

Hmm, I just realized this PR is currently missing a history.PushState. If you change the hash to get to a different snippet, going "back" should take you to the previous snippet.

@dmitshur
Copy link
MemberAuthor

Never mind, that's already taken care of. The browser does it by default in this situation.

image

Pressing back/forward allows you to navigate to previous snippets.

So yeah, I think it's good to merge.

@dmitshur
Copy link
MemberAuthor

If there are no objections, I'll merge this tomorrow.

I think it's a clear improvement over current behavior, and I'm not seeing any issues so far.

dmitshur added a commit that referenced this pull request Jan 16, 2015
…hange playground: Reload snippet on hash change.
@dmitshurdmitshur merged commit 9792975 into masterJan 16, 2015
@dmitshurdmitshur deleted the feature/reload-snippet-on-hash-change branch January 16, 2015 06:28
@dmitshur
Copy link
MemberAuthor

Seems to work okay to me, not seeing any issues.

Note that the code needs updating from Str() to String(), but the PR worked okay because it contained already-compiled javascript.

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@dmitshur