|
|
|
playground: use HTML5 History on share, to update URL in browser UI
Patch Set 1 #Patch Set 2 : diff -r 781fa4f5d565 https://code.google.com/p/go-playground/ #Patch Set 3 : diff -r 781fa4f5d565 https://code.google.com/p/go-playground/ #Patch Set 4 : diff -r 781fa4f5d565 https://code.google.com/p/go-playground/ #
Total comments: 10
Patch Set 5 : diff -r 781fa4f5d565 https://code.google.com/p/go-playground/ #
Total comments: 1
Total messages: 8
|
sanjay.m
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/
|
13 years, 4 months ago (2012年09月16日 00:24:47 UTC) #1 |
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/
Hello golang-dev@googlegroups.com (cc: adg@golang.org, golang-dev@googlegroups.com), Please take another look.
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
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.
Hello golang-dev@googlegroups.com, adg@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
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
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
*** 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>