code review 14726043: go-tour: new tour UI

go-tour: new tour UI

[francesc, 2013-10-17 22:59:12]

This is now ready for review

[adg, 2013-10-25 14:41:45]

Is the latest version of this at http://campoy-test.appspot.com/ ? It looks
very nice.


On 18 October 2013 02:59, <campoy@golang.org> wrote:

> Reviewers: adg,
>
> Message:
> This is now ready for review
>
> Description:
> go-tour: new tour UI
>
> Please review this at
https://codereview.appspot.**com/14726043/<https://codereview.appspot.com/147...
>
> Affected files (+64303, -3967 lines):
>   M app.yaml
>   M gotour/appengine.go
>   M gotour/local.go
>   M gotour/tour.go
>   R js/codemirror/lib/codemirror.**js
>   R js/codemirror/lib/go.js
>   R js/lang.js
>   R js/tour.js
>   M template/action.tmpl
>   A template/index.tmpl
>   R template/tour.tmpl
>   A web/css/app.css
>   A web/img/favicon.ico
>   A web/img/gopher.png
>   A web/js/app.js
>   A web/js/controllers.js
>   A web/js/services.js
>   A web/lib/angular-ui/angular-ui.**css
>   A web/lib/angular-ui/angular-ui.**js
>   A web/lib/angular-ui/ui-**codemirror/ui-codemirror.js
>   A web/lib/angular-ui/ui-**codemirror/ui-codemirror.min.**js
>   A web/lib/angular/angular-**cookies.js
>   A web/lib/angular/angular-**cookies.min.js
>   A web/lib/angular/angular-**loader.js
>   A web/lib/angular/angular-**loader.min.js
>   A web/lib/angular/angular-**resource.js
>   A web/lib/angular/angular-**resource.min.js
>   A web/lib/angular/angular-**sanitize.js
>   A web/lib/angular/angular-**sanitize.min.js
>   A web/lib/angular/angular.js
>   A web/lib/angular/angular.min.js
>   A web/lib/angular/version.txt
>   A web/lib/bootstrap/css/**bootstrap-theme.css
>   A web/lib/bootstrap/css/**bootstrap-theme.min.css
>   A web/lib/bootstrap/css/**bootstrap.css
>   A web/lib/bootstrap/css/**bootstrap.min.css
>   A web/lib/bootstrap/fonts/**glyphicons-halflings-regular.**eot
>   A web/lib/bootstrap/fonts/**glyphicons-halflings-regular.**svg
>   A web/lib/bootstrap/fonts/**glyphicons-halflings-regular.**ttf
>   A web/lib/bootstrap/fonts/**glyphicons-halflings-regular.**woff
>   A web/lib/bootstrap/js/**bootstrap.js
>   A web/lib/bootstrap/js/**bootstrap.min.js
>   A web/lib/codemirror/AUTHORS
>   A web/lib/codemirror/LICENSE
>   A web/lib/codemirror/README.md
>   A web/lib/codemirror/addon/edit/**closebrackets.js
>   A web/lib/codemirror/addon/edit/**closetag.js
>   A web/lib/codemirror/addon/edit/**continuelist.js
>   A web/lib/codemirror/addon/edit/**matchbrackets.js
>   A web/lib/codemirror/addon/edit/**matchtags.js
>   A web/lib/codemirror/addon/edit/**trailingspace.js
>   A web/lib/codemirror/keymap/**emacs.js
>   A web/lib/codemirror/keymap/**extra.js
>   A web/lib/codemirror/keymap/vim.**js
>   A web/lib/codemirror/lib/**codemirror.css
>   A web/lib/codemirror/lib/**codemirror.js
>   A web/lib/codemirror/mode/go/go.**js
>   A web/lib/jquery-ui/index.html
>   A web/lib/jquery-ui/js/jquery-1.**9.1.js
>   A web/lib/jquery-ui/js/jquery-**ui-1.10.3.js
>   A web/lib/jquery-ui/js/jquery-**ui-1.10.3.min.js
>   A web/lib/jquery/jquery.min.js
>   A web/partials/editor.html
>   A web/partials/home.html
>   A web/partials/modules.html
>
>
>

[francesc, 2013-10-25 14:56:56]

Yes, that's the latest version. And thanks :)


On Fri, Oct 25, 2013 at 11:41 AM, Andrew Gerrand <adg@golang.org> wrote:

> Is the latest version of this at http://campoy-test.appspot.com/ ? It
> looks very nice.
>
>
> On 18 October 2013 02:59, <campoy@golang.org> wrote:
>
>> Reviewers: adg,
>>
>> Message:
>> This is now ready for review
>>
>> Description:
>> go-tour: new tour UI
>>
>> Please review this at
https://codereview.appspot.**com/14726043/<https://codereview.appspot.com/147...
>>
>> Affected files (+64303, -3967 lines):
>>   M app.yaml
>>   M gotour/appengine.go
>>   M gotour/local.go
>>   M gotour/tour.go
>>   R js/codemirror/lib/codemirror.**js
>>   R js/codemirror/lib/go.js
>>   R js/lang.js
>>   R js/tour.js
>>   M template/action.tmpl
>>   A template/index.tmpl
>>   R template/tour.tmpl
>>   A web/css/app.css
>>   A web/img/favicon.ico
>>   A web/img/gopher.png
>>   A web/js/app.js
>>   A web/js/controllers.js
>>   A web/js/services.js
>>   A web/lib/angular-ui/angular-ui.**css
>>   A web/lib/angular-ui/angular-ui.**js
>>   A web/lib/angular-ui/ui-**codemirror/ui-codemirror.js
>>   A web/lib/angular-ui/ui-**codemirror/ui-codemirror.min.**js
>>   A web/lib/angular/angular-**cookies.js
>>   A web/lib/angular/angular-**cookies.min.js
>>   A web/lib/angular/angular-**loader.js
>>   A web/lib/angular/angular-**loader.min.js
>>   A web/lib/angular/angular-**resource.js
>>   A web/lib/angular/angular-**resource.min.js
>>   A web/lib/angular/angular-**sanitize.js
>>   A web/lib/angular/angular-**sanitize.min.js
>>   A web/lib/angular/angular.js
>>   A web/lib/angular/angular.min.js
>>   A web/lib/angular/version.txt
>>   A web/lib/bootstrap/css/**bootstrap-theme.css
>>   A web/lib/bootstrap/css/**bootstrap-theme.min.css
>>   A web/lib/bootstrap/css/**bootstrap.css
>>   A web/lib/bootstrap/css/**bootstrap.min.css
>>   A web/lib/bootstrap/fonts/**glyphicons-halflings-regular.**eot
>>   A web/lib/bootstrap/fonts/**glyphicons-halflings-regular.**svg
>>   A web/lib/bootstrap/fonts/**glyphicons-halflings-regular.**ttf
>>   A web/lib/bootstrap/fonts/**glyphicons-halflings-regular.**woff
>>   A web/lib/bootstrap/js/**bootstrap.js
>>   A web/lib/bootstrap/js/**bootstrap.min.js
>>   A web/lib/codemirror/AUTHORS
>>   A web/lib/codemirror/LICENSE
>>   A web/lib/codemirror/README.md
>>   A web/lib/codemirror/addon/edit/**closebrackets.js
>>   A web/lib/codemirror/addon/edit/**closetag.js
>>   A web/lib/codemirror/addon/edit/**continuelist.js
>>   A web/lib/codemirror/addon/edit/**matchbrackets.js
>>   A web/lib/codemirror/addon/edit/**matchtags.js
>>   A web/lib/codemirror/addon/edit/**trailingspace.js
>>   A web/lib/codemirror/keymap/**emacs.js
>>   A web/lib/codemirror/keymap/**extra.js
>>   A web/lib/codemirror/keymap/vim.**js
>>   A web/lib/codemirror/lib/**codemirror.css
>>   A web/lib/codemirror/lib/**codemirror.js
>>   A web/lib/codemirror/mode/go/go.**js
>>   A web/lib/jquery-ui/index.html
>>   A web/lib/jquery-ui/js/jquery-1.**9.1.js
>>   A web/lib/jquery-ui/js/jquery-**ui-1.10.3.js
>>   A web/lib/jquery-ui/js/jquery-**ui-1.10.3.min.js
>>   A web/lib/jquery/jquery.min.js
>>   A web/partials/editor.html
>>   A web/partials/home.html
>>   A web/partials/modules.html
>>
>>
>>
>


-- 
--
@francesc

[adg, 2013-10-28 05:28:32]

Some initial comments, mostly around naming.

I met a guy here in Istanbul who has a lot of expeirnece with Angular and I
might ask him to take a look, if that's ok with you.

https://codereview.appspot.com/14726043/diff/96001/gotour/local.go
File gotour/local.go (right):

https://codereview.appspot.com/14726043/diff/96001/gotour/local.go#newcode102
gotour/local.go:102: webFs := http.FileServer(http.Dir(filepath.Join(root,
"web")))
what's the distinction between /web/ and /static/?

https://codereview.appspot.com/14726043/diff/96001/gotour/local.go#newcode118
gotour/local.go:118: http.HandleFunc("/module/", func(w http.ResponseWriter, r
*http.Request) {
please make this handler a separate function

https://codereview.appspot.com/14726043/diff/96001/template/action.tmpl
File template/action.tmpl (right):

https://codereview.appspot.com/14726043/diff/96001/template/action.tmpl#newco...
template/action.tmpl:30: {{/* code is not displayed in the slides */}}
hmmm.. it should be possible to still use .code directives in the slides I think

https://codereview.appspot.com/14726043/diff/96001/web/js/app.js
File web/js/app.js (right):

https://codereview.appspot.com/14726043/diff/96001/web/js/app.js#newcode5
web/js/app.js:5: angular.module('gotour', ['ui', 'gotour.services',
'gotour.controllers', 'ng']).
can we call it 'tour' ?

https://codereview.appspot.com/14726043/diff/96001/web/js/app.js#newcode8
web/js/app.js:8: when('/home', {templateUrl: 'partials/home.html'}).
what about just / ?

https://codereview.appspot.com/14726043/diff/96001/web/js/app.js#newcode9
web/js/app.js:9: when('/modules', {templateUrl: 'partials/modules.html'}).
maybe /list and list.html?

[adg, 2013-10-31 09:31:23]

I notice that you have duplicates of several javascript files (including the two
mentioned below, and the angular and codemirror files). Please remove the
non-minified versions of the javascript files from this CL

https://codereview.appspot.com/14726043/diff/96001/web/lib/jquery-ui/js/jquer...
File web/lib/jquery-ui/js/jquery-1.9.1.js (right):

https://codereview.appspot.com/14726043/diff/96001/web/lib/jquery-ui/js/jquer...
web/lib/jquery-ui/js/jquery-1.9.1.js:1: /*!
Why do we need this file when we already have the minified version?

https://codereview.appspot.com/14726043/diff/96001/web/lib/jquery-ui/js/jquer...
File web/lib/jquery-ui/js/jquery-ui-1.10.3.js (right):

https://codereview.appspot.com/14726043/diff/96001/web/lib/jquery-ui/js/jquer...
web/lib/jquery-ui/js/jquery-ui-1.10.3.js:1: /*! jQuery UI - v1.10.3 - 2013-09-26
Why do we need this file when we already have the minified version?

[adg, 2013-10-31 09:33:53]

https://codereview.appspot.com/14726043/diff/96001/web/partials/editor.html
File web/partials/editor.html (right):

https://codereview.appspot.com/14726043/diff/96001/web/partials/editor.html#n...
web/partials/editor.html:15: <a href="" ng-click="lastPage()">&gt;&gt;</a>
why would you ever want to navigate to the first or last page? First I can maybe
understand, but this could be solved with a table of contents. Is there a table
of contents? I can't find it. (The current tour has one, click the "Go" tab in
the top right.)

[adg, 2013-11-04 05:55:18]

Please make this your priority this week.
I will do my best to respond to code reviews requests as quickly as
possible.

[francesc, 2013-11-07 22:59:20]

PTAL

Working on the TOC of slides in a module.

https://codereview.appspot.com/14726043/diff/96001/gotour/local.go
File gotour/local.go (right):

https://codereview.appspot.com/14726043/diff/96001/gotour/local.go#newcode102
gotour/local.go:102: webFs := http.FileServer(http.Dir(filepath.Join(root,
"web")))
On 2013/10/28 05:28:33, adg wrote:
> what's the distinction between /web/ and /static/?

Removed static and move the contents to either web or content/img

https://codereview.appspot.com/14726043/diff/96001/gotour/local.go#newcode118
gotour/local.go:118: http.HandleFunc("/module/", func(w http.ResponseWriter, r
*http.Request) {
On 2013/10/28 05:28:33, adg wrote:
> please make this handler a separate function

Done.

https://codereview.appspot.com/14726043/diff/96001/template/action.tmpl
File template/action.tmpl (right):

https://codereview.appspot.com/14726043/diff/96001/template/action.tmpl#newco...
template/action.tmpl:30: {{/* code is not displayed in the slides */}}
On 2013/10/28 05:28:33, adg wrote:
> hmmm.. it should be possible to still use .code directives in the slides I
think

Ok, modified so .play is not displayed (that's the playable code on the left
side), but the .code is kept in place.

Note that multiple .play in a slide create multiple tabs on the left pane :-)

https://codereview.appspot.com/14726043/diff/96001/web/js/app.js
File web/js/app.js (right):

https://codereview.appspot.com/14726043/diff/96001/web/js/app.js#newcode5
web/js/app.js:5: angular.module('gotour', ['ui', 'gotour.services',
'gotour.controllers', 'ng']).
On 2013/10/28 05:28:33, adg wrote:
> can we call it 'tour' ?

Done.

https://codereview.appspot.com/14726043/diff/96001/web/js/app.js#newcode8
web/js/app.js:8: when('/home', {templateUrl: 'partials/home.html'}).
On 2013/10/28 05:28:33, adg wrote:
> what about just / ?

Done.

https://codereview.appspot.com/14726043/diff/96001/web/js/app.js#newcode9
web/js/app.js:9: when('/modules', {templateUrl: 'partials/modules.html'}).
On 2013/10/28 05:28:33, adg wrote:
> maybe /list and list.html?

Done.

https://codereview.appspot.com/14726043/diff/96001/web/js/app.js#newcode9
web/js/app.js:9: when('/modules', {templateUrl: 'partials/modules.html'}).
On 2013/10/28 05:28:33, adg wrote:
> maybe /list and list.html?

Done.

https://codereview.appspot.com/14726043/diff/96001/web/lib/jquery-ui/js/jquer...
File web/lib/jquery-ui/js/jquery-1.9.1.js (right):

https://codereview.appspot.com/14726043/diff/96001/web/lib/jquery-ui/js/jquer...
web/lib/jquery-ui/js/jquery-1.9.1.js:1: /*!
On 2013/10/31 09:31:24, adg wrote:
> Why do we need this file when we already have the minified version?

removed

https://codereview.appspot.com/14726043/diff/96001/web/lib/jquery-ui/js/jquer...
File web/lib/jquery-ui/js/jquery-ui-1.10.3.js (right):

https://codereview.appspot.com/14726043/diff/96001/web/lib/jquery-ui/js/jquer...
web/lib/jquery-ui/js/jquery-ui-1.10.3.js:1: /*! jQuery UI - v1.10.3 - 2013-09-26
On 2013/10/31 09:31:24, adg wrote:
> Why do we need this file when we already have the minified version?

removed

https://codereview.appspot.com/14726043/diff/96001/web/partials/editor.html
File web/partials/editor.html (right):

https://codereview.appspot.com/14726043/diff/96001/web/partials/editor.html#n...
web/partials/editor.html:15: <a href="" ng-click="lastPage()">&gt;&gt;</a>
On 2013/10/31 09:33:54, adg wrote:
> why would you ever want to navigate to the first or last page? First I can
maybe
> understand, but this could be solved with a table of contents. Is there a
table
> of contents? I can't find it. (The current tour has one, click the "Go" tab in
> the top right.)

There's no table of contents right now and I'm not sure if we should have it.

In my view, once modules are split into small pieces there should be around 5 or
6 slides per module, therefore no need of table of contents. Maybe I'm wrong,
though

[adg, 2013-11-07 23:04:02]

On 8 November 2013 09:59, <campoy@golang.org> wrote:

> In my view, once modules are split into small pieces there should be
> around 5 or 6 slides per module, therefore no need of table of contents.
> Maybe I'm wrong, though
>

It's a relatively simple feature to add and I think
a) the new design should work with the current tour
b) it's not unlikely that modules might be tens of pages long

[adg, 2013-11-07 23:04:39]

On 8 November 2013 09:59, <campoy@golang.org> wrote:

> Working on the TOC of slides in a module.


Please let me know when this is ready and I'll take another look.

[francesc, 2013-11-08 05:46:41]

On 2013/11/07 23:04:39, adg wrote:
> On 8 November 2013 09:59, <mailto:campoy@golang.org> wrote:
> 
> > Working on the TOC of slides in a module.
> 
> 
> Please let me know when this is ready and I'll take another look.

After lots of fighting and learning I think this is pretty good.

I split the big controller in two and moved most of the logic to a service.

[adg, 2013-11-11 01:17:26]

On 2013/11/07 22:59:20, gocampoy wrote:
> Removed static and move the contents to either web or content/img

I would really prefer this was all still under "static". "web" is pretty
meaningless.

[adg, 2013-11-11 01:24:15]

What happened to the page numbers?

Why is there such a long delay between clicking "Try the first lesson" and the
content appearing?

The front page is underwhelming. There's nothing there to compel the user to
move forward. The current state is to start with some code. I think we should
try the same approach here. Start with hello world (the existing tour) and make
access to the other modules convenient

[adg, 2013-11-11 01:26:10]

It's bad that on the pages with no code the code box is there, editable, but
empty and with no filename tab.

It should be disabled or something.

[adg, 2013-11-11 02:10:50]

https://codereview.appspot.com/14726043/diff/382001/gotour/tour.go
File gotour/tour.go (right):

https://codereview.appspot.com/14726043/diff/382001/gotour/tour.go#newcode114
gotour/tour.go:114: // TODO: generate the source based on the name of the
module.
why is this a todo?

[adg, 2013-11-14 00:04:16]

https://codereview.appspot.com/14726043/diff/382001/gotour/local.go
File gotour/local.go (right):

https://codereview.appspot.com/14726043/diff/382001/gotour/local.go#newcode103
gotour/local.go:103: webFs := http.FileServer(http.Dir(filepath.Join(root,
"web")))
just make these global variables

https://codereview.appspot.com/14726043/diff/382001/gotour/local.go#newcode103
gotour/local.go:103: webFs := http.FileServer(http.Dir(filepath.Join(root,
"web")))
s/webFs/web/

https://codereview.appspot.com/14726043/diff/382001/gotour/local.go#newcode107
gotour/local.go:107: http.HandleFunc("/", rootHandler(webFs, contentFs))
don't pass in the handlers here; just use globals and make rootHandler a regular
function (not a function that returns a function)

https://codereview.appspot.com/14726043/diff/382001/gotour/tour.go
File gotour/tour.go (right):

https://codereview.appspot.com/14726043/diff/382001/gotour/tour.go#newcode31
gotour/tour.go:31: sync.RWMutex
just load all the modules at tour startup; don't worry about the lock

https://codereview.appspot.com/14726043/diff/382001/gotour/tour.go#newcode81
gotour/tour.go:81: content, err := parseModule(name)
just do this in initTour and delete the module function.

https://codereview.appspot.com/14726043/diff/382001/gotour/tour.go#newcode91
gotour/tour.go:91: // File defines the JSON format for a code file in a page.
s/format for/form of/g

https://codereview.appspot.com/14726043/diff/382001/gotour/tour.go#newcode93
gotour/tour.go:93: Name    string `json:"name"`
why lowercase? why not just make the json fields the same as the struct fields?

https://codereview.appspot.com/14726043/diff/382001/gotour/tour.go#newcode212
gotour/tour.go:212: func servePlayground() error {
why isn't this just an init() function?

[adg, 2013-11-14 00:18:17]

It's going to take more than one CL to land this. I think we should work in
 a new repository until this is ready, and then patch it over to the main
repo in one fell swoop.

I've created https://code.google.com/p/go-tour-redesign
Please edit your go-tour/.hg/hgrc file to point to this new repository, so
that when you submit this CL the change is submitted there instead of the
main go-tour repository.

Andrew

[francesc, 2013-11-14 14:07:00]

PTAL, last version live on http://campoy.cat/gotour

https://codereview.appspot.com/14726043/diff/382001/gotour/local.go
File gotour/local.go (right):

https://codereview.appspot.com/14726043/diff/382001/gotour/local.go#newcode103
gotour/local.go:103: webFs := http.FileServer(http.Dir(filepath.Join(root,
"web")))
On 2013/11/14 00:04:17, adg wrote:
> just make these global variables

Done.

https://codereview.appspot.com/14726043/diff/382001/gotour/local.go#newcode103
gotour/local.go:103: webFs := http.FileServer(http.Dir(filepath.Join(root,
"web")))
On 2013/11/14 00:04:17, adg wrote:
> s/webFs/web/

renamed as staticHandler, since static is already an imported package name.

https://codereview.appspot.com/14726043/diff/382001/gotour/local.go#newcode107
gotour/local.go:107: http.HandleFunc("/", rootHandler(webFs, contentFs))
On 2013/11/14 00:04:17, adg wrote:
> don't pass in the handlers here; just use globals and make rootHandler a
regular
> function (not a function that returns a function)

Done.

https://codereview.appspot.com/14726043/diff/382001/gotour/tour.go
File gotour/tour.go (right):

https://codereview.appspot.com/14726043/diff/382001/gotour/tour.go#newcode31
gotour/tour.go:31: sync.RWMutex
On 2013/11/14 00:04:17, adg wrote:
> just load all the modules at tour startup; don't worry about the lock

Done.

https://codereview.appspot.com/14726043/diff/382001/gotour/tour.go#newcode81
gotour/tour.go:81: content, err := parseModule(name)
On 2013/11/14 00:04:17, adg wrote:
> just do this in initTour and delete the module function.

Done.

https://codereview.appspot.com/14726043/diff/382001/gotour/tour.go#newcode91
gotour/tour.go:91: // File defines the JSON format for a code file in a page.
On 2013/11/14 00:04:17, adg wrote:
> s/format for/form of/g

Done.

https://codereview.appspot.com/14726043/diff/382001/gotour/tour.go#newcode93
gotour/tour.go:93: Name    string `json:"name"`
On 2013/11/14 00:04:17, adg wrote:
> why lowercase? why not just make the json fields the same as the struct
fields?

Done, but it makes the javascript code non idiomatic.

https://codereview.appspot.com/14726043/diff/382001/gotour/tour.go#newcode114
gotour/tour.go:114: // TODO: generate the source based on the name of the
module.
On 2013/11/11 02:10:51, adg wrote:
> why is this a todo?

Done.

https://codereview.appspot.com/14726043/diff/382001/gotour/tour.go#newcode212
gotour/tour.go:212: func servePlayground() error {
On 2013/11/14 00:04:17, adg wrote:
> why isn't this just an init() function?

Done.

[francesc, 2013-11-14 14:07:03]

PTAL, last version live on http://campoy.cat/gotour

[adg, 2013-11-15 02:47:16]

The styles in the TOC are pretty ugly.

[image: Inline images 1]


On 15 November 2013 01:07, <campoy@golang.org> wrote:

> PTAL, last version live on http://campoy.cat/gotour
>
> https://codereview.appspot.com/14726043/
>

[adg, 2013-11-15 02:47:42]

This is starting to look very nice.

[francesc, 2013-11-18 09:08:35]

PTAL

[adg, 2013-11-19 03:24:54]

https://codereview.appspot.com/14726043/diff/432001/template/index.tmpl
File template/index.tmpl (right):

https://codereview.appspot.com/14726043/diff/432001/template/index.tmpl#newcode8
template/index.tmpl:8: <link rel="stylesheet" type="text/css"
href="/lib/angular-ui/angular-ui.css">
these files are no longer present in the cl

[francesc, 2013-11-19 09:39:41]

https://codereview.appspot.com/14726043/diff/432001/template/index.tmpl
File template/index.tmpl (right):

https://codereview.appspot.com/14726043/diff/432001/template/index.tmpl#newcode8
template/index.tmpl:8: <link rel="stylesheet" type="text/css"
href="/lib/angular-ui/angular-ui.css">
On 2013/11/19 03:24:55, adg wrote:
> these files are no longer present in the cl

Done.

[adg, 2013-11-20 06:16:11]

On 2013/11/19 09:39:41, gocampoy wrote:
> https://codereview.appspot.com/14726043/diff/432001/template/index.tmpl
> File template/index.tmpl (right):
> 
>
https://codereview.appspot.com/14726043/diff/432001/template/index.tmpl#newcode8
> template/index.tmpl:8: <link rel="stylesheet" type="text/css"
> href="/lib/angular-ui/angular-ui.css">
> On 2013/11/19 03:24:55, adg wrote:
> > these files are no longer present in the cl
> 
> Done.

Not done.

[francesc, 2013-11-20 15:14:56]

On 2013/11/20 06:16:11, adg wrote:
> On 2013/11/19 09:39:41, gocampoy wrote:
> > https://codereview.appspot.com/14726043/diff/432001/template/index.tmpl
> > File template/index.tmpl (right):
> > 
> >
>
https://codereview.appspot.com/14726043/diff/432001/template/index.tmpl#newcode8
> > template/index.tmpl:8: <link rel="stylesheet" type="text/css"
> > href="/lib/angular-ui/angular-ui.css">
> > On 2013/11/19 03:24:55, adg wrote:
> > > these files are no longer present in the cl
> > 
> > Done.
> 
> Not done.

Weird. Uploaded again, the change was already there.

[adg, 2013-11-20 23:41:14]

https://codereview.appspot.com/14726043/diff/472001/template/index.tmpl
File template/index.tmpl (right):

https://codereview.appspot.com/14726043/diff/472001/template/index.tmpl#newco...
template/index.tmpl:38: <script src="/lib/angular-ui/angular-ui.js"></script>
this stuff is still not in the cl
are you looking at the codereview ui?

[adg, 2013-11-20 23:43:00]

The last time it was present is patch set 21
https://codereview.appspot.com/14726043/#ps382001

Have you made sure all the files are in the CL? see "hg p" and "hg status". They
must be "hg add"ed and then "hg file"d.

[francesc, 2013-11-21 17:38:31]

https://codereview.appspot.com/14726043/diff/472001/template/index.tmpl
File template/index.tmpl (right):

https://codereview.appspot.com/14726043/diff/472001/template/index.tmpl#newco...
template/index.tmpl:38: <script src="/lib/angular-ui/angular-ui.js"></script>
On 2013/11/20 23:41:15, adg wrote:
> this stuff is still not in the cl
> are you looking at the codereview ui?

I wonder how that happened. This should be good now. Sorry.

[adg, 2013-11-22 03:18:18]

https://codereview.appspot.com/14726043/diff/532001/static/js/app.js
File static/js/app.js (right):

https://codereview.appspot.com/14726043/diff/532001/static/js/app.js#newcode1
static/js/app.js:1: 'use strict';
all the original javascript files need copyright headers

[adg, 2013-11-22 03:29:50]

LGTM

After you've addressed my previous comments let's check this in (not to the main
repo) and move from there.

https://codereview.appspot.com/14726043/diff/532001/static/js/services.js
File static/js/services.js (right):

https://codereview.appspot.com/14726043/diff/532001/static/js/services.js#new...
static/js/services.js:5: angular.module('tour.services', []);
huh? needs a comment

[adg, 2013-11-22 03:29:50]

LGTM

After you've addressed my previous comments let's check this in (not to the main
repo) and move from there.

https://codereview.appspot.com/14726043/diff/532001/static/js/services.js
File static/js/services.js (right):

https://codereview.appspot.com/14726043/diff/532001/static/js/services.js#new...
static/js/services.js:5: angular.module('tour.services', []);
huh? needs a comment

[francesc, 2013-11-22 12:22:23]

Done

https://codereview.appspot.com/14726043/diff/532001/static/js/app.js
File static/js/app.js (right):

https://codereview.appspot.com/14726043/diff/532001/static/js/app.js#newcode1
static/js/app.js:1: 'use strict';
On 2013/11/22 03:18:20, adg wrote:
> all the original javascript files need copyright headers

Done.

https://codereview.appspot.com/14726043/diff/532001/static/js/services.js
File static/js/services.js (right):

https://codereview.appspot.com/14726043/diff/532001/static/js/services.js#new...
static/js/services.js:5: angular.module('tour.services', []);
On 2013/11/22 03:29:52, adg wrote:
> huh? needs a comment

Removed, not needed.

[francesc, 2013-11-22 12:35:08]

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

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

[francesc, 2013-11-22 12:36:11]

*** Submitted as
https://code.google.com/p/go-tour-redesign/source/detail?r=ff98a47a05d7 ***

go-tour: new tour UI

R=adg
CC=golang-dev
https://codereview.appspot.com/14726043

[adg, 2013-11-25 03:55:41]

Are you using a modified version of the present package?

https://codereview.appspot.com/14726043/diff/572001/gotour/tour.go
File gotour/tour.go (right):

https://codereview.appspot.com/14726043/diff/572001/gotour/tour.go#newcode133
gotour/tour.go:133: if err := sec.Render(w, tmpl); err != nil {
./tour.go:133: sec.Render undefined (type present.Section has no field or method
Render)

https://codereview.appspot.com/14726043/diff/572001/gotour/tour.go#newcode143
gotour/tour.go:143: f.Content = string(c.Raw)
./tour.go:143: c.Raw undefined (type *present.Code has no field or method Raw)

[francesc, 2013-11-25 08:11:25]

Yes, see http://codereview.appspot.com/26390044.


On Mon, Nov 25, 2013 at 4:55 AM, <adg@golang.org> wrote:

> Are you using a modified version of the present package?
>
>
> https://codereview.appspot.com/14726043/diff/572001/gotour/tour.go
> File gotour/tour.go (right):
>
> https://codereview.appspot.com/14726043/diff/572001/
> gotour/tour.go#newcode133
> gotour/tour.go:133: if err := sec.Render(w, tmpl); err != nil {
> ./tour.go:133: sec.Render undefined (type present.Section has no field
> or method Render)
>
> https://codereview.appspot.com/14726043/diff/572001/
> gotour/tour.go#newcode143
> gotour/tour.go:143: f.Content = string(c.Raw)
> ./tour.go:143: c.Raw undefined (type *present.Code has no field or
> method Raw)
>
> https://codereview.appspot.com/14726043/
>



-- 
--
@francesc