Fix ToC scrolls off screen on articles (close #258) #292

Merged
merged 8 commits into from Apr 2, 2014

3 participants

@vicb

@sethladd @kwalrath This fixes ToC scrolling has promised in the original issue #258.

A few remarks:

  • I had to patch bootstrap.js, see the first commit comment for details (fix coming in bs3.1.0),
  • I took this opportunity to harmonize things (Polymer ToC now looks like all other ToC, ie background color),
  • As a bonus I had a scroll spy to highlight the current section in the ToC (w/ a bold font).

The L&F should now be consistent across /articles, /docs and /polymer

@vicb

Rebased on master.

You can see it online https://vbe-dartlang.appspot.com/

@vicb

@sethladd do you consider merging this ?

@sethladd
Dart member

I tried https://vbe-dartlang.appspot.com/docs/dart-up-and-running/contents/ch02.html#variables-optional-types but the header is still under the nav. Should it work there?

@vicb

This one is about scrolling the TOC on articles and other pages

@vicb

@sethladd you were right, this PR also fixes the header overriding the anchor target... but not on the book. It works everywhere but on the up & running book. You can merge this anyway, I'll check what's wrong there.

@vicb vicb added a commit to vicb/dartlang.org that referenced this pull request Mar 4, 2014
@vicb vicb Update Bootstrap to v3.1.1
Bootstrap 3.1 is required for #292 which rely on twbs/bootstrap#11199
being fixed (the PR contain a patched version of bootstrap).
9ac4be9
@vicb vicb referenced this pull request Mar 4, 2014
Closed

Update Bootstrap to v3.1.1 #308

@kwalrath
Dart member

I wouldn't worry too much about the book. We don't intend to keep it in that format for too much longer.

@vicb

that's most probably only a css selector to update anyway... I only need to find the 5 required minutes to do this :) but the PR can be merged w/o this - the issue is here today w/ the book.

@kwalrath
Dart member

The book format is funny because it's an automatically translated thing. It has other CSS issues, like the TOC formatting (iirc) and the links in the footer. So... If it's easy, go for it! But as long as this PR doesn't break the book, I wouldn't hold up this PR.

@kwalrath
Dart member

Oops, it looks like I dropped the ball here. @sethladd and @vicb is it OK to merge this?

@vicb

OK

@kwalrath
Dart member

I just noticed that in https://vbe-dartlang.appspot.com/tools/pub/, the TOC is over the title.

@kwalrath
Dart member

Ah, it's because the pub pages (and maybe others) have this:

# {{ page.title }}

{% include toc.html %}

Instead of this:

{% include toc.html %}

# {{ page.title }}
@vicb

Thanks for a detailed review again!

Should I fix this or do you handle it ?

@kwalrath
Dart member

I can probably do it... I'm looking to see how widespread it is, first.

@vicb

Thanks !
Do not hesitate to ping me if you need my help.

@kwalrath
Dart member

This page has a problem, too, due to an extremely long TOC:

https://vbe-dartlang.appspot.com/support/faq.html

How can we fix that?

@kwalrath
Dart member

This page should scroll but doesn't:

https://vbe-dartlang.appspot.com/samples/

It's a generated TOC. Is there an easy way to fix it? (It doesn't look bad, but it'd be much better if it scrolled.)

@vicb

I'm not really sure there is a simple way to fix this... we would need to scroll based on the position in the page which would be much more complex.

Can't we deal with the current display ? we can see the bottom part of the TOC by scrolling down.

(I'll check if the scrollspy could help here)

@vicb

We could probably do something with the scrollspy but that would require a bit of thinking + extra JS, do you think it's worth it ?

@vicb

My last commit fix the pub pages, any other page to consider in tools ?

Working on the sample, should be ready soon.

@vicb

The samples page should be fixed now (online)

@vicb

@kwalrath actually I think the tools guys want to have the title full width and the ToC below it. It would help if you can indicate on which pages you noticed the issue. I'll fix that

@vicb

@kwalrath conclusion:

  • Scrolling ToCs have to use a fixed offset for the screen top
  • Then it's not possible to put a header with a "random" height on full content width,
  • The best would be to re-write the tools pages as what I've done for pub (pls indicate wich other ones I've missed)
  • The benefit is that the design is coherent across all pages.
@kwalrath
Dart member

I can't automatically merge this. Could you update your branch to have the latest dartlang.org files?

vicb added some commits Jan 22, 2014
@vicb vicb AFFIXed the ToC
Had to hotpatch bs, bad but should be safe, see
twbs/bootstrap#11199 for details (scheduled for
3.1.0)

Remaining issues:
- does not work on /docs yet,
- does not work for small window widths,
- sidebar width depends on content width, should be fixed, f(screen
width)
8f5ce61
@vicb vicb fix positiong problem (articles) 211a5c8
@vicb vicb AFFIXed docs section 9aa09f9
@vicb vicb fix all remaining issues ! c24ea65
@vicb vicb Add a scrollspy to highlight the current section ff06dda
@vicb vicb Oops ! dc9f759
@vicb vicb fix ToC on pub pages 5cda5fa
@vicb vicb Scroll the ToC on the samples page 1a736d5
@vicb

I would have expected more conflicts after a month... only 1 trivial, cool !

The new version is being upload on vbe-...

@kwalrath kwalrath merged commit b73f469 into dart-lang:master Apr 2, 2014
@kwalrath
Dart member

We noticed one title missing from one of the pub pages, but @Sfshaza will fix that. Thanks, Vic, for sticking with this PR for so long!

@vicb vicb deleted the vicb:ToC branch Apr 3, 2014
@sethladd sethladd referenced this pull request in dart-lang/dartdoc Jan 16, 2015
Closed

[Feature] Implement a scroll spy in the navigator #115

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment