make the book more mobile friendly

[sfaxon]

Helps with mobile friendliness of The Rust Book #20850

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. The way Github handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see CONTRIBUTING.md for more information.

[Gankra]

Can you host a build for review?

[Gankra]

This is an... interesting... approach. I feel like reading from a file proper would make more sense, but I'll admit I'm new to this specific codebase.

[sfaxon]

I borrowed using a module from the way the CSS is implemented:
https://github.com/rust-lang/rust/blob/master/src/rustbook/css.rs

I agree it is an "interesting" approach, but since it's my first pull request I tried to stick to what I thought was convention.

[Gankra]

@aturon what's up with this? Quick hack or something you stand by?

[aturon]

I have zero recollection of this -- I suspect it was borrowed from rustdoc. In any case, all of rustbook is a very quick hack and I'm happy for it to be improved.

[sfaxon]

@aturon Where is the authoritative rustbook source? https://github.com/rust-lang/rust/tree/master/src/rustbook or https://github.com/aturon/rust-book?

[aturon]

We're trying to move to
https://github.com/rust-lang/rust/tree/master/src/rustbook

On Tue, Jan 13, 2015 at 10:22 AM, Seth Faxon [email protected]
wrote:

In src/rustbook/javascript.rs
#21071 (diff):

+document.addEventListener("DOMContentLoaded", function(event) {

• document.getElementById("toggle-nav").onclick = toggleNav;
• function toggleNav() {
• var toc = document.getElementById("toc");
• var pagewrapper = document.getElementById("page-wrapper");
• if ( "block" === toc.style.display ) {

•  toc.style.display = "none";
  

•  pagewrapper.style.display = "block";
  

• } else {

•  toc.style.display = "block";
  

•  pagewrapper.style.display = "none";
  

• }
• };
  +});
  +</script>
  +"#;

@aturon https://github.com/aturon Where is the authoritative rustbook
source? https://github.com/rust-lang/rust/tree/master/src/rustbook or
https://github.com/aturon/rust-book?

—
Reply to this email directly or view it on GitHub
https://github.com/rust-lang/rust/pull/21071/files#r22882104.

[sfaxon]

Sorry, not sure what you mean by "host a build for review."

[Gankra]

Build the docs with these changes, and host them on the internet for us to poke at.

[Gankra]

It's fairly hard to review some CSS/JS tweaks in isolation like this.

[sfaxon]

Of course, I'm serving the generated Rust Book here: http://home.faxon.org/book/index.html. It's my best guess at replicating what http://doc.rust-lang.org/book/ is doing.

When the window size goes below 1060px it will switch to a mobile view and provides a button in the upper left to toggle showing the table of contents. The div that contains the toggle button is always there. If we want it to go away I would probably want to use a .js library. I'm happy to make that change as well, but I would want whoever is currently managing the docs site to weigh in on that.

I've tested on Android, iOS, Firefox, Safai, Chrome (Mac) and the latest IE. It behaves the same, just let me know if the behavior is desired :)

[Gankra]

I think the padding is a bit much for the <1060 view. I'm not an expert, but my understanding is that you basically want no padding for mobile.

[Gankra]

I think you can do some magix with css classes to get the hamburger to always do the right thing on mobile/desktop:

• Have the hamurger conditionally visible on the mobile size-class.
• Have the hamburger toggle a .mobile-hidden (or whatever) class on the content and TOC.
• Have the CSS hide .mobile-hidden only when the view is mobile-size.

This should make the hamburger only visible when the display is small, and only have effects when the view is large. As it stands now, it unreversible makes only one of the TOC and content visible, even when the view is made large again.

[Gankra]

Also is the 1060 value based on anything (some kind of hardware/layout standard?) or just eyeballed as "looks good"?

[sfaxon]

You are correct, a 15px padding would be better for the margins on mobile. I'll make that change.

The CSS is using media query to toggle showing the hamburger. You should only see the hamburger when the table of contents is hidden in condensed mode. Pressing the hamburger toggle does get into a toggle mode that does not leave when the browser expands. I can add some more one-off JavaScript to address it (most (all?) browsers don't honor media queries on resize), but it is starting to feel like using some sort of responsive framework would be helpful, but I don't think that's my decision to make.
Sorry, I was completely mistaken. Not sure where I got the idea that browsers didn't adjust media queries on resize. I just tested on Chrome, Firefox and Safari and they all work. I'll push that change as well.

I collapsed the table at 1060px because the existing copy width was set to 750px and the table of contents at 250px. Those widths plus margins = 1060px. And eyeballed as looks good :). Most frameworks collapse around 1000px for a tablet version and again somewhere around 600 or 700px. Since the Rust Book is mostly code in pre elements I wanted to lean towards hiding the nav on tablet to give longer lines of code room to be seen without sidescrolling.

[Gankra]

I might be missing something, but I'm pretty sure that the only JS you need is to toggle a single class onclick. Everything else should be reasonably handled by CSS.

e.g.

document.addEventListener("DOMContentLoaded", function(event) {
 document.getElementById("toggle-nav").onclick = toggleNav;

 function toggleNav() {
     var toc = document.getElementById("toc");
     var pagewrapper = document.getElementById("page-wrapper");

     toggleClass(toc, "mobile-hidden");
     toggleClass(pagewrapper, "mobile-hidden";)
 };

 function toggleClass(el, className) {
    // Stolen from http://youmightnotneedjquery.com/
    if (el.classList) {
      el.classList.toggle(className);
    } else {
      var classes = el.className.split(' ');
      var existingIndex = classes.indexOf(className);

      if (existingIndex >= 0) {
        classes.splice(existingIndex, 1);
      } else {
        classes.push(className);
      }

      el.className = classes.join(' ');
    }
 }
});

and

@media only screen and (max-width: 1060px) {
    .mobile-hidden { display: none }
}

[sfaxon]

@gankro I've update the pull request to toggle a class in order to hide the div, as you laid out. I think the issue I was having was related to setting display none in CSS on an ID rather than on a class. I also updated the page margin to be 15px on mobile. The code on http://home.faxon.org/book/ reflects the changes as well.

[Gankra]

Yeah, nice! This looks good to me with minor indentation fixes.

Would prefer someone else with rustbook/doc experience chime in on the string-litteral hack (and other design concerns).

[sfaxon]

@gankro Thanks for the feedback, and help! If there is a better way to implement the CSS and JavaScript importing I'm happy to take care of it.

[Gankra]

After you fix the indentation in the JS, feel free to squash the commits. I have a feeling this will get merged as-is, but just want to discuss design with others a bit beforehand.

[killercup]

@sfaxon I want to bikeshed this some more, I hope you don't mind :)

First off, you should probably set an explicit padding for #toggle-nav. Webkit applies a default of padding: 3px 6px; to button, but I'm not sure about other browsers.

Why are the bars 90% wide? 100% looks more symmetrical:

(I changed the first button's .bar styles to use width: 100%; padding: 0; margin: 3px 0 2px;.)

Last but not least, you should probably add a description of what that button does. A title would be nice for people not recognizing that it means "menu"/"navigation". Additionally, a textual representation for screen readers would improve accessibility quite a bit (I guess the text-only pages are otherwise accessible). Bootstrap uses this in their navbar (cf. docs): <span class="sr-only">Toggle navigation</span>.

[steveklabnik]

I'm fine with the Rustbook changes here.

[steveklabnik]

I tried this out on my phone, and the hamburger looks weird. It's got very, very skinny lines. (iPhone 6.)

[steveklabnik]

Other than that it looks great though!

[sfaxon]

I've addressed the CSS and screen reader changes from @killercup (thanks!), pushed changes here and redeployed to http://home.faxon.org/book/index.html for review. If everyone is ok with them I can squash before merging.

[killercup]

Looks great on my iPhone 5S.

Scrolling is a bit weird, though (it has no momentum). It might be nice to try -webkit-overflow-scrolling: touch; (some info), but that's probably not necessary right now.

[steveklabnik]

I think I'm happy with this, it's at least a strict improvement. So r=me after a squash

[sfaxon]

r= @steveklabnik

[Gankra]

@sfaxon Only reviewers can r= things :)

I already r+'d it myself, though.

[sfaxon]

@gankro sorry, I'm not familiar with the r notation. Is that documented somewhere that I didn't see?

[steveklabnik]

@sfaxon https://github.com/rust-lang/rust/wiki/Note-bors-usage

[iKevinY]

Great PR @sfaxon! We should definitely look to reimplement momentum-based scrolling as per @killercup's suggestion, as it plays a pretty large role in the browsing experience.

[dancek]

Hi everyone! What's the status on this PR? I noticed the same issues while reading the doc, and was planning to fix them but this seems to take care of it. Is this just waiting to be merged?

I do notice that toggling the navigation could be done using CSS3 :target only with no JS, but that's a minor detail. I also might have some opinions about the CSS on desktop, but I'd like to wait until this is merged before I touch anything. Mostly I'd like to know if enhancements to the web sites are welcome at this point.

[killercup]

@dancek:

Is this just waiting to be merged?

Yes, it's in bors' queue (quite a bit down currently, just search for the 21071).