code review 6498135: playground: use HTML5 History on share, to update URL i...

playground: use HTML5 History on share, to update URL in browser UI

[sanjay.m, 2012-09-16 00:24:47]

Hello golang-dev@googlegroups.com (cc: adg@golang.org,
golang-dev@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go-playground/

[sanjay.m, 2012-09-16 00:25:16]

Hello golang-dev@googlegroups.com (cc: adg@golang.org,
golang-dev@googlegroups.com),

Please take another look.

[adg, 2012-09-16 06:05:40]

Looks pretty good.

http://codereview.appspot.com/6498135/diff/8001/static/playground.js
File static/playground.js (right):

http://codereview.appspot.com/6498135/diff/8001/static/playground.js#newcode19
static/playground.js:19: // these checks also suggest we have addEventListener
why not check also? it's shorter than the comment and will always be correct

http://codereview.appspot.com/6498135/diff/8001/static/playground.js#newcode20
static/playground.js:20: if (window.history && window.history.pushState &&
!!opts['enableHistory']) {
is the !! necessary?

http://codereview.appspot.com/6498135/diff/8001/static/playground.js#newcode20
static/playground.js:20: if (window.history && window.history.pushState &&
!!opts['enableHistory']) {
put this if statement after the two function declarations that it references.

http://codereview.appspot.com/6498135/diff/8001/static/playground.js#newcode27
static/playground.js:27: function inputChanged() {
Please make this function also hide shareEl, so that the user doesn't keep
seeing an out-of-date share url.

http://codereview.appspot.com/6498135/diff/8001/static/playground.js#newcode41
static/playground.js:41: setBody(e.state.code);
also this should be below setBody. I know js let's you call declarations that
haven't been made yet, but the results can be surprising. let's keep it in order
please

[sanjay.m, 2012-09-16 06:50:49]

PTAL

https://codereview.appspot.com/6498135/diff/8001/static/playground.js
File static/playground.js (right):

https://codereview.appspot.com/6498135/diff/8001/static/playground.js#newcode19
static/playground.js:19: // these checks also suggest we have addEventListener
On 2012/09/16 06:05:40, adg wrote:
> why not check also? it's shorter than the comment and will always be correct

Done.

https://codereview.appspot.com/6498135/diff/8001/static/playground.js#newcode20
static/playground.js:20: if (window.history && window.history.pushState &&
!!opts['enableHistory']) {
On 2012/09/16 06:05:40, adg wrote:
> put this if statement after the two function declarations that it references.

Done.

https://codereview.appspot.com/6498135/diff/8001/static/playground.js#newcode20
static/playground.js:20: if (window.history && window.history.pushState &&
!!opts['enableHistory']) {
On 2012/09/16 06:05:40, adg wrote:
> is the !! necessary?

Done.

https://codereview.appspot.com/6498135/diff/8001/static/playground.js#newcode27
static/playground.js:27: function inputChanged() {
On 2012/09/16 06:05:40, adg wrote:
> Please make this function also hide shareEl, so that the user doesn't keep
> seeing an out-of-date share url.

Done.

https://codereview.appspot.com/6498135/diff/8001/static/playground.js#newcode41
static/playground.js:41: setBody(e.state.code);
On 2012/09/16 06:05:40, adg wrote:
> also this should be below setBody. I know js let's you call declarations that
> haven't been made yet, but the results can be surprising. let's keep it in
order
> please

Done.

[sanjay.m, 2012-09-16 06:51:07]

Hello golang-dev@googlegroups.com, adg@golang.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[sanjay.m, 2012-09-16 06:54:59]

This is off-topic, but there's a call to insertTabs with 2 arguments. Looks 
like a typo to me. I can fix this while I'm here if it is.

Sanjay

[adg, 2012-09-17 17:04:45]

LGTM

https://codereview.appspot.com/6498135/diff/12002/static/playground.js
File static/playground.js (right):

https://codereview.appspot.com/6498135/diff/12002/static/playground.js#newcode47
static/playground.js:47: insertTabs(tabs, 1);
yep, this is a bug. i will fix in a followup cl

[adg, 2012-09-17 17:05:04]

*** Submitted as
http://code.google.com/p/go-playground/source/detail?r=037317d4acb3 ***

playground: use HTML5 History on share, to update URL in browser UI

R=golang-dev, adg
CC=golang-dev
http://codereview.appspot.com/6498135

Committer: Andrew Gerrand <adg@golang.org>