Extensions Docs Server: Highlight links on left nav

Extensions Docs Server: Highlight links on left nav

The link for the page you are on will now be hightlighted in the left nav bar.

BUG=142008
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=151516

[cduvall, 2012-08-13 22:27:55]

Quick fix for sidenav highlighting.

[not at google - send to devlin, 2012-08-13 23:17:21]

http://codereview.chromium.org/10827304/diff/2001/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/static/css/site.css (right):

http://codereview.chromium.org/10827304/diff/2001/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/static/css/site.css:347: cursor: default;
cursor styling should be unnecssary if we just remove the href attribute (see
comment in the js file).

http://codereview.chromium.org/10827304/diff/2001/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/static/js/sidebar.js (right):

http://codereview.chromium.org/10827304/diff/2001/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/static/js/sidebar.js:62: var selectedNode
= null;
nit: some separation between steps here would be nice. Like, above "var links
=",

// Figure out which of the links has been selected so that we can style it
differently.

http://codereview.chromium.org/10827304/diff/2001/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/static/js/sidebar.js:64: if (links[i].href
== document.location.href) {
href isn't quite right. If somebody navigates directly to an anchor then that
will be reflected in document.location.href, but not in links[i].href.

Better perhaps would be to compare links[i].getAttribute('href') (should be a
path) versus document.location.pathname.

also, nit: do

if (!condition)
  continue;
...
break;

rather than

if (condition) {
  ...
  break;
}

http://codereview.chromium.org/10827304/diff/2001/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/static/js/sidebar.js:66: links[i].href =
'javascript:void(0);';
if you do removeAttribute('href') it would convey the intent more clearly I
think, and you wouldn't need to manually disable the link styling in CSS.

http://codereview.chromium.org/10827304/diff/2001/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/static/js/sidebar.js:71: var items =
toc.getElementsByTagName('li');
nit: as above,

// Blah.

http://codereview.chromium.org/10827304/diff/2001/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/static/js/sidebar.js:74: if
(item.firstChild && item.firstChild.tagName == 'SPAN') {
if (!item.firstChild || item.firstChild.tagName != 'SPAN')
  continue;

?

[cduvall, 2012-08-13 23:46:10]

https://chromiumcodereview.appspot.com/10827304/diff/2001/chrome/common/exten...
File chrome/common/extensions/docs/server2/static/css/site.css (right):

https://chromiumcodereview.appspot.com/10827304/diff/2001/chrome/common/exten...
chrome/common/extensions/docs/server2/static/css/site.css:347: cursor: default;
On 2012/08/13 23:17:21, kalman wrote:
> cursor styling should be unnecssary if we just remove the href attribute (see
> comment in the js file).

Done.

https://chromiumcodereview.appspot.com/10827304/diff/2001/chrome/common/exten...
File chrome/common/extensions/docs/server2/static/js/sidebar.js (right):

https://chromiumcodereview.appspot.com/10827304/diff/2001/chrome/common/exten...
chrome/common/extensions/docs/server2/static/js/sidebar.js:62: var selectedNode
= null;
On 2012/08/13 23:17:21, kalman wrote:
> nit: some separation between steps here would be nice. Like, above "var links
> =",
> 
> // Figure out which of the links has been selected so that we can style it
> differently.

Done.

https://chromiumcodereview.appspot.com/10827304/diff/2001/chrome/common/exten...
chrome/common/extensions/docs/server2/static/js/sidebar.js:64: if (links[i].href
== document.location.href) {
On 2012/08/13 23:17:21, kalman wrote:
> href isn't quite right. If somebody navigates directly to an anchor then that
> will be reflected in document.location.href, but not in links[i].href.
> 
> Better perhaps would be to compare links[i].getAttribute('href') (should be a
> path) versus document.location.pathname.
> 
> also, nit: do
> 
> if (!condition)
>   continue;
> ...
> break;
> 
> rather than
> 
> if (condition) {
>   ...
>   break;
> }

Done.

https://chromiumcodereview.appspot.com/10827304/diff/2001/chrome/common/exten...
chrome/common/extensions/docs/server2/static/js/sidebar.js:66: links[i].href =
'javascript:void(0);';
On 2012/08/13 23:17:21, kalman wrote:
> if you do removeAttribute('href') it would convey the intent more clearly I
> think, and you wouldn't need to manually disable the link styling in CSS.

Done.

https://chromiumcodereview.appspot.com/10827304/diff/2001/chrome/common/exten...
chrome/common/extensions/docs/server2/static/js/sidebar.js:71: var items =
toc.getElementsByTagName('li');
On 2012/08/13 23:17:21, kalman wrote:
> nit: as above,
> 
> // Blah.

Done.

https://chromiumcodereview.appspot.com/10827304/diff/2001/chrome/common/exten...
chrome/common/extensions/docs/server2/static/js/sidebar.js:74: if
(item.firstChild && item.firstChild.tagName == 'SPAN') {
On 2012/08/13 23:17:21, kalman wrote:
> if (!item.firstChild || item.firstChild.tagName != 'SPAN')
>   continue;
> 
> ?

Done.

[not at google - send to devlin, 2012-08-14 04:07:20]

lgtm

http://codereview.chromium.org/10827304/diff/6002/chrome/common/extensions/do...
File chrome/common/extensions/docs/server2/static/js/sidebar.js (right):

http://codereview.chromium.org/10827304/diff/6002/chrome/common/extensions/do...
chrome/common/extensions/docs/server2/static/js/sidebar.js:66: var path_parts =
document.location.pathname.split('/')
what about if the path ends with some number of /s.

[cduvall, 2012-08-14 18:23:22]

https://chromiumcodereview.appspot.com/10827304/diff/6002/chrome/common/exten...
File chrome/common/extensions/docs/server2/static/js/sidebar.js (right):

https://chromiumcodereview.appspot.com/10827304/diff/6002/chrome/common/exten...
chrome/common/extensions/docs/server2/static/js/sidebar.js:66: var path_parts =
document.location.pathname.split('/')
On 2012/08/14 04:07:20, kalman wrote:
> what about if the path ends with some number of /s.

Done.