Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add rustdoc documentation complexity toggle #16169

Merged
merged 1 commit into from
Aug 2, 2014

Conversation

Gankra
Copy link
Contributor

@Gankra Gankra commented Aug 1, 2014

Adds a simple/detailed toggle to api doc pages.
Detailed mode is the current behaviour, simple mode hides all doccomment details leaving only signatures for quick browsing.

Adds [expand all] and [collapse all] "links" to all api doc pages. All doccomments are collapsed, leaving only signatures for quick browsing.

In addition, clicking on a function name function's [toggle details] link now toggles the visibility of the associated doccomment.


Live Build Here

This is something that's been bothering me, and I've seen some people mention in IRC before. The docs are great if you want a full in-depth look at an API, but awful if you want to scan them. This provides the ability to toggle complexity freely. Interacts perfectly well with noscript, since the static page is effectively unchanged. Collapsing is just hiding divs with css.

I'm not much of a designer, so design input welcome on the actual UX for toggling.

The actual javascript is a bit brittle to layout changes, but it always will be without adding lots of extra junk to the actual markup, which didn't seem worth it.

r##"<span id='render-detail'>
<button class='pill left inactive' id='render-simple'>simple</button><button
class='pill right active' id='render-detailed'>detailed</button>
</span>"##));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To all reviewers: the buttons are line-broken in this weird way to avoid any whitespace between the two buttons, which would actually render.

@huonw
Copy link
Member

huonw commented Aug 1, 2014

FWIW, I couldn't tell which side of the pill button was activated and which side was not until clicking on it several times. (random idea: it could just be a [ ] summary checkbox.)

function renderDocsSimple(){
$("#render-detailed").addClass("inactive").removeClass("active");
$("#render-simple").addClass("active").removeClass("inactive");
$(".docblock").hide();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couldn't this be done in CSS? i.e. add class to some parent element with a rule .summary-mode .docblock { display: none; } (this could also be used to control the styling of the buttons).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The nice thing about my current design is you can individually hide/reveal the blocks by clicking on the function name. I don't think that works with your suggestion.

@Gankra
Copy link
Contributor Author

Gankra commented Aug 1, 2014

@huonw: I find your suggestion about a checkbox interesting. In fact, the more I think about it, you aren't even entering/exiting a state, you're just calling "collapse all" and "expand all".

@Gankra
Copy link
Contributor Author

Gankra commented Aug 1, 2014

Refactored the thing to just have [collapse all] [expand all] "links" which is a much better UX in my mind. Also drastically reduces the amount of stuff this commit does.

@steveklabnik
Copy link
Member

This is super interesting. I tweeted about it, let's see what some other people say.

@steveklabnik
Copy link
Member

Said tweet is here: https://twitter.com/steveklabnik/status/495248942767288320

there's a couple of responses so far

@aaroncm
Copy link

aaroncm commented Aug 1, 2014

(expanding on my tweet response)
I like the idea of being able to see just the signatures, but I'm not sure about that UI. Maybe something like "View: Detailed | Brief" instead of expand/collapse? It would probably be helpful if there were some visual indication of what could be expanded/collapsed, as well, rather than it being a 'hidden' behavior of the method names.

@nham
Copy link
Contributor

nham commented Aug 1, 2014

If it's a toggle, we don't need to show both links at any time, right? Could we show only "Expand" when its collapsed and only "Collapse" when its expanded?

@Gankra
Copy link
Contributor Author

Gankra commented Aug 1, 2014

@nham: it's no longer a toggle (well, it never really was, in practice). You should always be able to individually expand/collapse them individually.

@Gankra
Copy link
Contributor Author

Gankra commented Aug 1, 2014

Made individual toggles explicit distinct entity.

@lilyball
Copy link
Contributor

lilyball commented Aug 1, 2014

Those links are HUGE. Also, I was really hoping to get a subtle [src] link on each function in exactly the place you put the toggle.

Is this kind of toggle really what we want anyway? I always figured we should have an expandable index of all functions at the top, to allow you to jump to a function.

@lilyball
Copy link
Contributor

lilyball commented Aug 1, 2014

I suppose with everything collapsed the whole page kind of acts like an index. But it would be nice if the individual toggles could be much more subtle. They could also perhaps even go on the left side, in the margin between the stability indicator and the fn. Maybe a nice small [-] / [+] link?

@nham
Copy link
Contributor

nham commented Aug 1, 2014

Yeah, the expansion links are probably going to have to go on the left, otherwise you get something like this, which is what it currently looks like on my machine: rust_expand_details_links_too_big

@thehydroimpulse
Copy link
Contributor

Yeah, the links are way too big. It should also really be a single link (at the top) that changes based on the state. Otherwise it brings too much noise to the mix.

@Gankra
Copy link
Contributor Author

Gankra commented Aug 1, 2014

Moved link to the left, made subtler. Working on statefully toggling label between [+] and [-]

@Gankra
Copy link
Contributor Author

Gankra commented Aug 1, 2014

Aaaand stateful. Alright, this looks a lot better now. Thanks for the feedback, everyone!

@lilyball
Copy link
Contributor

lilyball commented Aug 1, 2014

That looks better. The margin changes when I toggle the button now though; it needs to have a fixed width. I'd also recommend making it a grey instead of a black.

@Gankra
Copy link
Contributor Author

Gankra commented Aug 1, 2014

@kballard: the padding doesn't change, it's just that + and - don't have the same width in the font used. Exploring monospace possibilities right now. Good call on grey, though.

@nham
Copy link
Contributor

nham commented Aug 1, 2014

When one function has details and an adjacent one doesn't, the function signatures aren't aligned (but probably should be). Example:

Example

@lilyball
Copy link
Contributor

lilyball commented Aug 1, 2014

@gankro The [-] and [+] should not be affecting the positioning of the function signature.

@lilyball
Copy link
Contributor

lilyball commented Aug 1, 2014

The toggle right now uses a margin-left: -20px to counteract the stability's margin, and then adds a padding-right. If you want to do it that way, it should use a block layout and set its width explicitly to 20px (with no margin/padding) to ensure it takes up exactly the same amount of space. Then you can fiddle with the text inside to make sure it never strays out of this 20px space.

@Gankra
Copy link
Contributor Author

Gankra commented Aug 1, 2014

@kballard I opted for position:absolute so that it has no influence over the positioning of its neighbours, solving both alignment issues.

@lilyball
Copy link
Contributor

lilyball commented Aug 1, 2014

Better. Still 2 nitpicks:

  1. The width of the [-] / [+] itself still changes when it toggles. It would be great to fix this. Presumably that means fixing the width of - to be the same as the width of +, which I'm not sure offhand how to do without switching to a monospace font. But perhaps switching to a monospace font is the right way to do this.
  2. The collapse/expand animation is a little off. It seems like the animation is actually vertically offset down a few pixels. Most obviously, this causes the animation to appear to believe the description is taller than it is (it "jumps" to a larger size before collapsing, and when expanding, it "jumps" to a smaller size when done). But less obviously, the closed end of the animation is also off; there's a slight hitch when it finishes closing where it appears to settle on a particular height and then immediately "jump" to an even shorter height.

The animation issues reproduce in both Safari and Chrome.

@Gankra
Copy link
Contributor Author

Gankra commented Aug 1, 2014

Any animation issues are just a jQuery problem, as far as I know. I can just axe them, if you want (I'm just a sucker for sliding). I think what you're describing might be on purpose. E.G. they're trying to make it "bounce". I'll need to review their docs on slideUp/slideDown.

Making +/- monospace seems like the right call. It just looks but-ugly if you make the [] parts monospace, too. Gonna have to make a sub-span, I guess.

@lilyball
Copy link
Contributor

lilyball commented Aug 1, 2014

Can you make the whole thing monospace, like the function signature, so we can see what it looks like?

@Gankra
Copy link
Contributor Author

Gankra commented Aug 1, 2014

@kballard: done. I also tweaked the animation parameters to make it snappier. (changes uncommited)

@lilyball
Copy link
Contributor

lilyball commented Aug 1, 2014

Snappier is good, but now I see what you mean about looking ugly. It's much wider than expected, and there's more space to the right of the -/+ than to the left.

@lilyball
Copy link
Contributor

lilyball commented Aug 1, 2014

What if you leave it in the original font, and just use a sub-span for the -/+ with display: inline-block that sets width: 1ch or somesuch? (I'm not sure what the right unit is, I didn't even realize ch was a unit but apparently that's the width of 0, other units typically represent height).

@Gankra
Copy link
Contributor Author

Gankra commented Aug 1, 2014

@kballard: that worked perfectly when combined with text-align:center :D

@Gankra
Copy link
Contributor Author

Gankra commented Aug 1, 2014

@tinaun: good catch on browser specific stuff. Fixed that at least. Will address traits when I get back.

@kballard: a [+/-] below the struct declaration seems like it would be fine. I'll mock it out when I get back.

@Gankra
Copy link
Contributor Author

Gankra commented Aug 1, 2014

Alright, I think that's all the issues addressed!

@lilyball
Copy link
Contributor

lilyball commented Aug 1, 2014

Looks pretty good. I'm not entirely sold on having that [-] just floating above the primary page documentation, but I don't have a better solution.

I'm also not sure that indenting trait methods is correct when there's no impl block for it. It looks ok when there's a [-] indicator, but when there's not it's kind of odd. Not odd enough to say it's wrong, but still odd.

At this point, I have no more concrete suggestions. I'll let someone else actually decide if this is appropriate though (I do not wish to claim any authority over how our documentation should look). Although I will say that you're going to have to squash your commits.

@Gankra
Copy link
Contributor Author

Gankra commented Aug 1, 2014

Yeah indenting traits isn't perfect but I don't think it's worth entering elaborate case analysis for indentation-or-not behavior.

Yep, I'll squash commits as soon as I get a peseudo-r on this. Who has any kind of authority on this matter? @cmr? @steveklabnik?

@steveklabnik
Copy link
Member

I like it, but I'm not sure I have that authority. @huonw , what do you think?

@emberian
Copy link
Member

emberian commented Aug 1, 2014

I'm happy with this, would r+

@ftxqxd
Copy link
Contributor

ftxqxd commented Aug 1, 2014

I was thinking that the toggle could be in the margin, like this:

Toggle in the margin

And then would show a short comment when collapsed:

Short comment

But right now it’s OK. Maybe that could be done in a separate PR.

@Gankra
Copy link
Contributor Author

Gankra commented Aug 1, 2014

@cmr: Everything squashed

@emberian
Copy link
Member

emberian commented Aug 1, 2014

@gankro can you implement @P1start's recommendation?

@tinaun
Copy link
Contributor

tinaun commented Aug 1, 2014

The only complaint I have with this at the moment is that the toggles move into an awkward place when dealing with long struct names + smaller browser widths. I don't really know what the best way to handle small widths would be, however.

Again, this could be saved for a later change .

@emberian
Copy link
Member

emberian commented Aug 2, 2014

r=me if you do.

@sinistersnare
Copy link
Contributor

As an aside: thank you so much, I love you ❤️

@Gankra
Copy link
Contributor Author

Gankra commented Aug 2, 2014

Alright, how's that look? @cmr @P1start

@lilyball
Copy link
Contributor

lilyball commented Aug 2, 2014

It shows "Expand Description" too soon. During the collapse it's overlapping the text. Perhaps it can crossfade between the text and "Expand Description"?

Also the width of the toggle on the description is too small.

And there's a new bug where collapsing a function documentation doesn't update the toggle, until you expand and collapse again.

Edit: I thought a second toggle fixed it, but now it appears that it's strictly the inverse of how it should (except it initializes to the correct value).

@Gankra
Copy link
Contributor Author

Gankra commented Aug 2, 2014

Definitely in the kind of groggy state that produced the initial commit for this thing... Everything's gonna be down-hill from here, guys. Lets make some UX

@ftxqxd
Copy link
Contributor

ftxqxd commented Aug 2, 2014

The ‘Expand Description’ message could have a tiny bit more padding underneath it, and changed to sentence case (i.e., ‘Expand description’, not ‘Expand Description’).

Edit: Ideally the crossfade animation wouldn’t play when the ‘[collapse all]’ is pressed (because the slide animation doesn’t play either).

@Gankra
Copy link
Contributor Author

Gankra commented Aug 2, 2014

Shout outs to the commit "muhh" which produces "[+] +" when you collapse. Quality level: ++ tier

@Gankra
Copy link
Contributor Author

Gankra commented Aug 2, 2014

Alright, that last commit seems to not be awful, but working out the padding is gonna require a serious refactor, to be honest. The toggle needs to change how much space it occupies to do it properly. Gonna go to sleep, and poke at this when I'm not the worst at coding.

@Gankra
Copy link
Contributor Author

Gankra commented Aug 2, 2014

Vastly more conscious now. How's it look now?

@tinaun
Copy link
Contributor

tinaun commented Aug 2, 2014

When you hide the description toggle, the entire thing jumps down a line before the animation starts.

@lilyball
Copy link
Contributor

lilyball commented Aug 2, 2014

Except for the issue @tinaun mentioned it looks good.

All doccomments are now collapsable via a nearby [-] button
Adds [collapse all] and [expand all] buttons to the top of all api pages
Tweaks some layout to accomadate this
@Gankra
Copy link
Contributor Author

Gankra commented Aug 2, 2014

More transitiony now. Good?

bors added a commit that referenced this pull request Aug 2, 2014
<strike>Adds a simple/detailed toggle to api doc pages.
Detailed mode is the current behaviour, simple mode hides all doccomment details leaving only signatures for quick browsing.
</strike>
Adds [expand all] and [collapse all] "links" to all api doc pages. All doccomments are collapsed, leaving only signatures for quick browsing.

In addition, clicking on a <strike>function name</strike> function's [toggle details] link now toggles the visibility of the associated doccomment.

--------

# [Live Build Here](http://cg.scs.carleton.ca/~abeinges/doc/std/vec/struct.Vec.html)

This is something that's been bothering me, and I've seen some people mention in IRC before. The docs are *great* if you want a full in-depth look at an API, but *awful* if you want to scan them. This provides the ability to toggle complexity freely. Interacts perfectly well with noscript, since the static page is effectively unchanged. Collapsing is just hiding divs with css.

I'm not much of a designer, so design input welcome on the actual UX for toggling.

The actual javascript is *a bit* brittle to layout changes, but it always will be without adding lots of extra junk to the actual markup, which didn't seem worth it.
@bors bors closed this Aug 2, 2014
@bors bors merged commit 88fe6df into rust-lang:master Aug 2, 2014
@Gankra
Copy link
Contributor Author

Gankra commented Aug 3, 2014

Awesome. Thanks for all the feedback guys! Much better than anything I could have come up with alone.

@Gankra Gankra deleted the simple-docs branch August 18, 2014 19:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.