Hide associated constants too when collapsing implementation #79796
Conversation
GuillaumeGomez
added
T-rustdoc
|
Some changes occurred in HTML/CSS/JS. |
rust-highfive
added
the
S-waiting-on-review
GuillaumeGomez
force-pushed
the
hide-associated-const-when-collapsing
branch
from
2d9d966
to
c277f37
Compare
|
@pickfire The force of habit. ;) |
|
@bors: r=pickfire rollup |
|
📌 Commit c277f3726d7b0bef72eb7d78349e14141e17ae99 has been approved by |
bors
added
S-waiting-on-bors
src/librustdoc/html/static/main.js
Outdated
| var shouldHide = hasClass(n, "method") || hasClass(n, "associatedconstant"); | ||
| if (shouldHide === true || fullHide === true || hasClass(n, "type")) { | ||
| if (shouldHide === true || fullHide === true) { |
There was a problem hiding this comment.
Why did you move out the check to fullHide here? There's no time you'd want to treat it separately from the method checks, right?
There was a problem hiding this comment.
Well, in the case it's an associated constant. ;)
There was a problem hiding this comment.
I don't understand what this means. I'm asking why shouldHide is treated separately from fullHide.
There was a problem hiding this comment.
I did noticed this part but both looks the same to me, either way looks fine but I prefer fullHide being part of shouldHide.
There was a problem hiding this comment.
It's a bit different: we don't hide "type" elements, only their doc block. So the first condition is either we should completely hide it or its doc block, the second one is if we should hide the element only.
There was a problem hiding this comment.
@GuillaumeGomez that's not my question. My question is why did you change from
var shouldHide = hasClass(n, "method") || hasClass(n, "associatedconstant") || fullHide;
if (shouldHide) { ... }to
var shouldHide = hasClass(n, "method") || hasClass(n, "associatedconstant");
if (shouldHide || fullHide) { ... }The second is much easier to forget the check to fullHide, and there's no need to make it separate.
There was a problem hiding this comment.
I thought on the contrary that it made the code easier to follow but I can put back fullHide into the variable.
|
@bors r- I would like to get an answer to #79796 (comment) before this merges (even if it's just explaining why the change happened). |
bors
added
S-waiting-on-author
GuillaumeGomez
force-pushed
the
hide-associated-const-when-collapsing
branch
from
c277f37
to
99df340
Compare
|
@jyn514 I have put back |
|
Forgot to r+ it... @bors: r=jyn514 rollup |
|
📌 Commit 99df340 has been approved by |
bors
added
S-waiting-on-bors
…laumeGomez Rollup of 6 pull requests Successful merges: - rust-lang#79379 (Show hidden elements by default when JS is disabled) - rust-lang#79796 (Hide associated constants too when collapsing implementation) - rust-lang#79958 (Fixes reported bugs in Rust Coverage) - rust-lang#80008 (Fix `cargo-binutils` link) - rust-lang#80016 (Use imports instead of rewriting the type signature of `RustcOptGroup::stable`) - rust-lang#80025 (Replace some `println!` with `tidy_error!` to simplify) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
Fixes #71849.
r? @jyn514