Migrate top doc and non-exhaustive toggles to details tag #85074
Conversation
GuillaumeGomez
added
T-rustdoc
|
Some changes occurred in HTML/CSS themes. Some changes occurred in HTML/CSS/JS. A change occurred in the Ayu theme. cc @Cldfire |
rust-highfive
added
the
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
|
Yay! Thanks for getting this across the finish line. Writing from my phone
so some brief things. Let's call the PR "migrate top doc and nom-exhaustive
toggles to details tag." I think that would be most informative to someone
reading through PRs later.
I see you've used the CSS "content" property to show the text of the
toggle. I know we've done this on one other toggle type but it makes me
uneasy. For instance someone searching for where a text string comes from
might look in the HTML or the JS, but not the CSS. I would prefer the
approach I took in the decl-hiding change: put the text (e.g. expand non
exhaustive) inside a `<summary class=hide-me>`.
Also, I see a lot of repetitive CSS selectors. I think we can reduce those
by giving our details tag the rustdoc-toggle class and selecting on that.
In general all our toggles should have that class in addition to any others
they have.
Also I see you've removed the `non-exhaustive` class in preference for more
specific classes. I'd like to keep it- I think it will make the CSS cleaner
and more maintainable.
I'm excited to see how much JS this lets us remove!
|
GuillaumeGomez
changed the title
GuillaumeGomez
force-pushed
the
end-toggle-migration
branch
from
913bfa4 to
c42c1dd
Compare
This comment has been minimized.
This comment has been minimized.
|
I see that document_full has a new boolean parameter, is_collapsible. I'm
always wary of boolean params because it's not clear at the call site what
they mean. How about this: at the one call site that has is_collapsible =
true, emit the opening and closing details tags there rather than inside
document_full?
Also the CSS rules for the [+] that were formerly split can be merged since
your latest updates.
|
It's problematic in case there isn't any documentation generated because then we'd have a |
GuillaumeGomez
force-pushed
the
end-toggle-migration
branch
from
c42c1dd to
fa8bfdc
Compare
This comment has been minimized.
This comment has been minimized.
GuillaumeGomez
force-pushed
the
end-toggle-migration
branch
from
fa8bfdc to
35cf559
Compare
camelid
changed the title
src/librustdoc/html/render/mod.rs
Outdated
| @@ -561,10 +561,21 @@ fn document_short( | |||
| } | |||
| } | |||
|
|
|||
| fn document_full(w: &mut Buffer, item: &clean::Item, cx: &Context<'_>) { | |||
| fn document_full(w: &mut Buffer, item: &clean::Item, cx: &Context<'_>, is_collapsible: bool) { | |||
There was a problem hiding this comment.
nit: Could you use an enum instead? document_full(w, item, cx, true) doesn't tell me what the boolean means and could be confusing.
enum Collapsible { True, False }
// Example usage
document_full(w, item, cx, Collapsible::True);Or, if you want to be able to use ifs, you could use a wrapper struct. This might be a better approach.
struct Collapsible { is: bool }
// Example call
document_full(w, item, cx, Collapsible { is: true });
// Example check
if collapsible.is { /* ... */ }I suggested naming the field is so that it's clear in the if conditional what the field is for.
There was a problem hiding this comment.
Creating a new type for a boolean seems completely overkill. The argument name is pretty explicit in itself I think. Here what I will do: create another function called document_full_collapsible so that it doesn't require an extra argument and it will be explicit what it's doing. :)
There was a problem hiding this comment.
The beauty of Rust's enums is that they're really easy to make and use. :-) But I'm good with document_full_collapsible if @camelid is.
Another possibility, going back to my idea of writing out the <details> wrapper at the call site: What if document_full returns an Option<Buffer>? Then the call sites can either splat the contents directly into their copy of w, or wrap them in <details> as appropriate.
There was a problem hiding this comment.
I didn't want to create a new Buffer, but that could work too, just not as performant.
camelid
added
the
C-cleanup
GuillaumeGomez
force-pushed
the
end-toggle-migration
branch
2 times, most recently
from
8eed102 to
987f5a6
Compare
GuillaumeGomez
force-pushed
the
end-toggle-migration
branch
2 times, most recently
from
7fd40ba to
b4dd386
Compare
|
Applied review comments: a huge reduction of the JS size: more than 200 lines removed! \o/ That's one of the biggest reduction we've seen I think. |
| hasClass(parent, "impl") === false) { | ||
| e.innerHTML = labelForToggleButton(true); | ||
| onEachLazy(document.getElementsByClassName("rustdoc-toggle"), function(e) { | ||
| if (e.parentNode.id !== "main" || |
There was a problem hiding this comment.
Why are direct children of main excluded from this?
There was a problem hiding this comment.
Only the type declaration <details> and the impl blocks are ignored in this case (look in the or condition ;) ), like it's currently the case. I kept the same logic.
GuillaumeGomez
force-pushed
the
end-toggle-migration
branch
from
b4dd386 to
3837c1c
Compare
|
@bors r+ Nice work! |
|
📌 Commit 3837c1c has been approved by |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
…tester, r=Mark-Simulacrum Improve rustdoc gui tester I cherry-picked the commit from rust-lang#84834 (and modified it a bit). I also used this opportunity to update it to last version (forgot to update GUI test in rust-lang#85074, really can't wait to make rust-lang#84586 finally work). cc `@Mark-Simulacrum` for the changes in bootstrap. r? `@jsha`
…, r=GuillaumeGomez Move global click handlers to per-element ones. In rustdoc's main.js, we had an onclick handler for the whole document that would dispatch to handlers for various elements. This change attaches the handlers to the elements that trigger them, instead. This simplifies the code and avoids reimplementing the browser's bubbling functionality. As part of this change, change from a class to an id for help button. Move the handlers and associated code for highlighting source lines into source-script.js (and factor out a shared regex). Demo at https://hoffman-andrews.com/rust/bubble-bubble-toil-and-trouble/std/string/struct.String.html Note: this conflicts with / depends on rust-lang#85074. Once that's merged I'll rebase this and resolve conflicts. Part of rust-lang#83332. Thanks to `@Manishearth` for the [suggestion to not reimplement bubbling](rust-lang#83332 (comment)). r? `@GuillaumeGomez`
Fixes #83332.
r? @jsha