Improve sidebar rendering and add methods list #45187
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Some changes occurred in HTML/CSS. |
|
(rust_highfive has picked a reviewer for you, use r? to override) |
GuillaumeGomez
force-pushed
the
doc-ui-improvement
branch
from
9811d99
to
9d010f5
Compare
kennytm
reviewed
Oct 11, 2017
src/librustdoc/html/render.rs
Outdated
| let ret = v.iter() | ||
| .filter(|i| i.inner_impl().trait_.is_none()) | ||
| .filter_map(|i| Some(get_methods(i.inner_impl()).join(""))) | ||
| .collect::<Vec<_>>().join(""); |
There was a problem hiding this comment.
- Isn't
.filter_map(|i| Some(...))equivalent to.map(|i| ...)? - You could
.collect::<String>()directly, instead of.collect::<Vec<_>>().join("")
So I guess what you want is
v.iter()
.filter(|i| i.inner_impl().trait_.is_none())
.flat_map(|i| get_methods(i.inner_impl()))
.collect::<String>();
src/librustdoc/html/render.rs
Outdated
| } | ||
| } else { | ||
| None | ||
| } |
There was a problem hiding this comment.
match item.name {
Some(ref name) if !name.is_empty() && item.visibility.is_some() && item.is_method() => {
Some(format!("<a href=\"#method.{name}\">{name}</a>", name = name))
}
_ => None,
}
|
In general, i'm not sure how i feel about it. It's something that's been requested before, so there's at least some demand there. But this has potential to make the sidebar profoundly confusing, especially on pages like Vec, in your example. If there's a ton of methods there, it's going to make the sidebar downright huge. This also completely buries any heading links that appear below the method listing, like "Methods from Deref" and "Trait Implementations". I would prefer to leave the links that were there at the top (since it was supposed to mock a table of contents), then print all the methods underneath those links. I would also like to see the Deref methods, since those are often as much a part of a type's API as its inherent methods. (Again, especially for popular types like Vec or String.) (This doubles the concern from earlier, though - it's going to be downright impossible to reach the bottom of Vec's or String's sidebar if all their Deref methods are there.) If you're going to make the sidebar headings no longer part of a list, do you want to do something similar to the dynamic sidebar content? I'm pretty sure i copied that structure from the "module siblings" listing that's loaded in dynamically. |
|
@QuietMisdreavus: I'm wondering about adding a way to expand/hide the list. Also, for the sidebar size: #45212 |
|
I really like the fact that this gives an easy to read overview of all of the method names. Right now if you are looking for something specific but don't remember how it's called, you have to scroll through the entire doc. I find that very impractical. |
To your point, types like Vec with tons of methods are specifically the cases where I most need an easy to read list of symbols, because the amount of stuff I need to otherwise scan through to find what I want is orders of magnitude larger than the side bar would be (no matter how big the latter is). |
aidanhs
added
the
S-waiting-on-review
GuillaumeGomez
force-pushed
the
doc-ui-improvement
branch
from
9d010f5
to
de1d4ec
Compare
|
Updated btw. |
GuillaumeGomez
added
I-nominated
A-docs
|
Okay, coming back to this, I'm much more willing to add it. Some non-code-nit concerns still stand, though:
|
That's why I suggested to add a "hide button", so like that, you call collapse the calls and you're good to go!
Hum sure. I was wondering if it'd be useful but if you think it is, then I can totally add it. |
|
We talked about this in the docs team meeting today, I suggested an alternate layout to @GuillaumeGomez and i'd like to see whether that works out. |
QuietMisdreavus
added
S-waiting-on-author
GuillaumeGomez
force-pushed
the
doc-ui-improvement
branch
2 times, most recently
from
c96bb7b
to
1fa3a28
Compare
| @@ -3606,15 +3640,15 @@ fn sidebar_trait(fmt: &mut fmt::Formatter, it: &clean::Item, | |||
|
|
|||
| sidebar.push_str("<li><a href=\"#implementors\">Implementors</a></li>"); | |||
There was a problem hiding this comment.
Move the sidebar_assoc_items call below these items and i'll call it good. As-is, this will print these links below the assoc items listing, thus putting the "table of contents" links between the methods listings and the module-siblings listings, effectively hiding them.
|
Cool! I'm willing to call it good at this point. r=me pending travis. |
kennytm
added
S-waiting-on-bors
|
Travis failed on linkchecker: Seems like the IDs for the trait link anchors aren't the same as the ones on the page. |
kennytm
added
S-waiting-on-author
|
Chrome and firefox are doing the translation by themselves? Interesting! |
kennytm
reviewed
Oct 28, 2017
src/librustdoc/html/render.rs
Outdated
| @@ -3206,12 +3206,37 @@ fn render_deref_methods(w: &mut fmt::Formatter, cx: &Context, impl_: &Impl, | |||
| } | |||
| } | |||
|
|
|||
| fn shoud_render_item(item: &clean::Item, deref_mut_: bool) -> bool { | |||
There was a problem hiding this comment.
I don't know if anyone cares but this should be spelled should_render_item 😐
(BTW why the underscore after deref_mut?)
|
⌛ Testing commit b712d516b3717535232b7267ad2d52eb1a699626 with merge 9573fcc79059466fce4883a3bfb8f542d45ef5cb... |
|
💔 Test failed - status-travis |
|
|
kennytm
added
S-waiting-on-author
GuillaumeGomez
force-pushed
the
doc-ui-improvement
branch
from
b712d51
to
6fa521c
Compare
|
Well, no clue what the error is about. Builds fine on my computer. Fixed the function name. |
|
@bors r=QuietMisdreavus Let's try again. If this turns out to be spurious, maybe we should disable MultipleCGU/ThinLTO on MIPS. |
|
📌 Commit 6fa521c has been approved by |
bors
added a commit
that referenced
this pull request
Oct 28, 2017
…reavus Improve sidebar rendering and add methods list I suppose it can be reviewed as is, but this is just the first step of a more global plan. cc @rust-lang/docs @nical And a screenshot of course: <img width="1440" alt="screen shot 2017-10-10 at 23 38 45" src="https://user-images.githubusercontent.com/3050060/31412170-657beaf6-ae14-11e7-9f01-1e562a034595.png">
|
💔 Test failed - status-travis |
|
Same error. Legit then. Did you recognize the following symbol mentioned in the linker error?
|
|
Maybe someone from the @rust-lang/compiler team will see the issue? |
This was referenced Oct 31, 2017
|
The linkage error here I hope will be unblocked with #45655 |
|
@bors: retry |
kennytm
added
S-waiting-on-bors
bors
added a commit
that referenced
this pull request
Nov 1, 2017
…reavus Improve sidebar rendering and add methods list I suppose it can be reviewed as is, but this is just the first step of a more global plan. cc @rust-lang/docs @nical And a screenshot of course: <img width="1440" alt="screen shot 2017-10-10 at 23 38 45" src="https://user-images.githubusercontent.com/3050060/31412170-657beaf6-ae14-11e7-9f01-1e562a034595.png">
|
☀️ Test successful - status-appveyor, status-travis |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I suppose it can be reviewed as is, but this is just the first step of a more global plan.
cc @rust-lang/docs @nical
And a screenshot of course: