code review 6631057: godoc: add dropdown playground to nav bar

godoc: add dropdown playground to nav bar

[adg, 2012-10-09 23:49:04]

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

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

[adg, 2012-10-09 23:50:14]

Here's how it looks.

[adg, 2012-10-09 23:51:20]

+skybrian

This also re-introduces the fixed nav bar on larger screens that was
previous rolled back. I solved the issue with the original version;
anchors links now lead to the right place.

On 10 October 2012 10:49,  <adg@golang.org> wrote:
> Reviewers: gri, dsymonds,
>
> Message:
> Hello gri@golang.org, dsymonds@golang.org (cc:
> golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> https://code.google.com/p/go
>
>
> Description:
> godoc: add dropdown playground to nav bar
>
> Please review this at http://codereview.appspot.com/6631057/
>
> Affected files:
>   M doc/godocs.js
>   M doc/style.css
>   M lib/godoc/godoc.html
>   M lib/godoc/package.html
>   M src/cmd/godoc/godoc.go
>
>

[dsymonds, 2012-10-09 23:56:06]

LGTM

not sure about the UI, but the code looks fine.

https://codereview.appspot.com/6631057/diff/11001/doc/godocs.js
File doc/godocs.js (right):

https://codereview.appspot.com/6631057/diff/11001/doc/godocs.js#newcode143
doc/godocs.js:143: if (!setup) {
maybe invert this and return early, outdenting the rest?

https://codereview.appspot.com/6631057/diff/11001/lib/godoc/godoc.html
File lib/godoc/godoc.html (right):

https://codereview.appspot.com/6631057/diff/11001/lib/godoc/godoc.html#newcode33
lib/godoc/godoc.html:33: <a id="playgroundButton" href="#" title="Show Go
Playground">Play</a>
unlike the other menu links, this doesn't go to a standalone page. can this not
just go to play.golang.org?

https://codereview.appspot.com/6631057/diff/11001/lib/godoc/godoc.html#newcode51
lib/godoc/godoc.html:51: <div class="output">Hello, 世界<pre>
having the output pre-populated means that a casual visitor hitting "Run" won't
see a change. I'm not sure this is a gain.

[adg, 2012-10-10 00:01:18]

https://codereview.appspot.com/6631057/diff/11001/doc/godocs.js
File doc/godocs.js (right):

https://codereview.appspot.com/6631057/diff/11001/doc/godocs.js#newcode143
doc/godocs.js:143: if (!setup) {
On 2012/10/09 23:56:06, dsymonds wrote:
> maybe invert this and return early, outdenting the rest?

Done.

https://codereview.appspot.com/6631057/diff/11001/lib/godoc/godoc.html
File lib/godoc/godoc.html (right):

https://codereview.appspot.com/6631057/diff/11001/lib/godoc/godoc.html#newcode33
lib/godoc/godoc.html:33: <a id="playgroundButton" href="#" title="Show Go
Playground">Play</a>
On 2012/10/09 23:56:06, dsymonds wrote:
> unlike the other menu links, this doesn't go to a standalone page. can this
not
> just go to play.golang.org?

Done.

Although the button will only appear for people that have javascript enabled,
and if they do it won't function as a link anyway.

https://codereview.appspot.com/6631057/diff/11001/lib/godoc/godoc.html#newcode51
lib/godoc/godoc.html:51: <div class="output">Hello, 世界<pre>
On 2012/10/09 23:56:06, dsymonds wrote:
> having the output pre-populated means that a casual visitor hitting "Run"
won't
> see a change. I'm not sure this is a gain.

Done.

[gri, 2012-10-10 00:06:54]

LGTM for the .go changes.

Can't speak for js.

[skybrian1, 2012-10-10 00:07:08]

I didn't see how the anchor problem was fixed. Is that related to the "wide"
class?

[adg, 2012-10-10 00:15:00]

Brian, see below

https://codereview.appspot.com/6631057/diff/13001/doc/style.css
File doc/style.css (right):

https://codereview.appspot.com/6631057/diff/13001/doc/style.css#newcode534
doc/style.css:534: position: fixed;
The trick is this. Making the #page be a fixed box that scrolls internally, with
#page .container taking the role of what #page used to be.

[adg, 2012-10-10 00:18:11]

*** Submitted as http://code.google.com/p/go/source/detail?r=f9f27066cb90 ***

godoc: add dropdown playground to nav bar

R=gri, dsymonds, skybrian
CC=golang-dev
http://codereview.appspot.com/6631057

[DMorsing, 2012-10-10 11:48:42]

On my 1080p monitor, the page div doesn't fill up the entire width of
the screen.

Screenshot attached.

[adg, 2012-10-10 21:58:27]

Thanks very much for the report. I've created
http://codereview.appspot.com/6643062

On 10 October 2012 22:48, Daniel Morsing <daniel.morsing@gmail.com> wrote:
> On my 1080p monitor, the page div doesn't fill up the entire width of
> the screen.
>
> Screenshot attached.