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

Migrate trait and impl blocks' toggles into #84754

Merged
merged 4 commits into from
May 2, 2021

Conversation

GuillaumeGomez
Copy link
Member

Part of #83332

After this, I think only the "global" doc comment will be used as JS toggle. Once this PR is merged, I check what remains and remove them.

There is one change that this PR brings:

Screenshot from 2021-04-30 15-39-04
Screenshot from 2021-04-30 15-39-07

As you can see, I had to move the "undocumented" items below, they're not mixed with the others anymore. Unfortunately, I don't see a way to keep the current appearance without JS. As a a reminder, currently it looks like this:

Screenshot from 2021-04-30 15-39-12
Screenshot from 2021-04-30 15-39-15

r? @jsha

@rust-highfive
Copy link
Collaborator

Some changes occurred in HTML/CSS themes.

cc @GuillaumeGomez

Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez

A change occurred in the Ayu theme.

cc @Cldfire

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Apr 30, 2021
@GuillaumeGomez GuillaumeGomez added T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. A-rustdoc-js Area: Rustdoc's JS front-end A-rustdoc-ui Area: Rustdoc UI (generated HTML) labels Apr 30, 2021
Copy link
Contributor

@jsha jsha left a comment

Choose a reason for hiding this comment

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

I think you need to add something around main.js line 1178, where we have showLargeItem / showImplementor, to make these toggles respond to their corresponding settings.

@GuillaumeGomez
Copy link
Member Author

Completely forgot about the settings, you're absolutely right, great catch!

src/librustdoc/html/render/mod.rs Show resolved Hide resolved
src/librustdoc/html/static/rustdoc.css Outdated Show resolved Hide resolved
src/librustdoc/html/static/rustdoc.css Outdated Show resolved Hide resolved
@@ -1810,3 +1833,14 @@ details.rustdoc-toggle[open] > summary::before {
content: "[−]";
display: inline;
}

details.undocumented > summary::before {
content: "[+] Show hidden undocumented items";
Copy link
Contributor

Choose a reason for hiding this comment

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

Interesting approach! This is different from how we've done the other toggles that have descriptions (hide-me), but it makes sense to do it this way, since this particular toggle has a description both in its open and in its closed state. 👍🏻

src/librustdoc/html/render/mod.rs Outdated Show resolved Hide resolved
src/librustdoc/html/render/mod.rs Outdated Show resolved Hide resolved
@GuillaumeGomez GuillaumeGomez force-pushed the toggle-migration branch 2 times, most recently from a6cb17d to 65a3e25 Compare May 1, 2021 19:33
@GuillaumeGomez
Copy link
Member Author

GuillaumeGomez commented May 1, 2021

Fixed the settings application and applied your comments. Please tell me what you think of how I applied your closure idea.

Comment on lines +1174 to +1184
if (hideMethodDocs === true) {
onEachLazy(document.getElementsByClassName("method"), function(e) {
var toggle = e.parentNode;
if (toggle) {
toggle = toggle.parentNode;
}
}
};

var funcImpl = function(e) {
var next = e.nextElementSibling;
if (next && hasClass(next, "item-info")) {
next = next.nextElementSibling;
}
if (next && hasClass(next, "docblock")) {
next = next.nextElementSibling;
}
if (!next) {
return;
}
};
if (toggle && toggle.tagName === "DETAILS") {
toggle.open = false;
}
});
}
Copy link
Contributor

@jsha jsha May 1, 2021

Choose a reason for hiding this comment

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

This would fit in more nicely as part of the onEachLazy(document.getElementsByTagName("details"), function (e) { loop below. That sections finds all the appropriate details tags and opens them if needed, which means we don't have to make the JS dependent on a very specific DOM structure by looking for a grandparent DETAILS node. Since methods are default-open, we would add a clause to say:

var hideMethod = hideMethods && hasClass(e, "methods-toggle");
if (hideMethod) {
  e.open = false;
}

Note that we also need the class methods-toggle in the emitted HTML.

Copy link
Member Author

Choose a reason for hiding this comment

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

So I'm shared here: I'm not a big fan of the idea to grow the HTML, but that would indeed improve the JS. So after giving it some thought, here is what I suggest: we will move the "method"/"function"/etc classes from the h3/h4 into the grand-parent details. However, it seems to be out of context here, so if you're fine with it: I'll open an issue so that it can be done later and not be forgotten.

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree HTML size is important, but I don't think we should economize by omitted classes when they are needed. That makes the CSS and JS more complex and harder to reason about. I'd rather look to bigger potential wins first - like deduplicating the tooltips subdocuments.

There's also an argument from consistency: We already put appropriate classes on the <details> tags we've added so far. It would be odd to economize on this one particular item type and not the others.

At any rate, I don't want to hold up the whole PR for the next week; I'm find landing this piece as-is and we can discuss more when I'm back.

src/librustdoc/html/static/rustdoc.css Outdated Show resolved Hide resolved
src/librustdoc/html/render/mod.rs Outdated Show resolved Hide resolved
src/librustdoc/html/render/mod.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@jsha jsha left a comment

Choose a reason for hiding this comment

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

r=me with the sense of the toggled variable inverted.

src/librustdoc/html/render/mod.rs Outdated Show resolved Hide resolved
Comment on lines +1174 to +1184
if (hideMethodDocs === true) {
onEachLazy(document.getElementsByClassName("method"), function(e) {
var toggle = e.parentNode;
if (toggle) {
toggle = toggle.parentNode;
}
}
};

var funcImpl = function(e) {
var next = e.nextElementSibling;
if (next && hasClass(next, "item-info")) {
next = next.nextElementSibling;
}
if (next && hasClass(next, "docblock")) {
next = next.nextElementSibling;
}
if (!next) {
return;
}
};
if (toggle && toggle.tagName === "DETAILS") {
toggle.open = false;
}
});
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I agree HTML size is important, but I don't think we should economize by omitted classes when they are needed. That makes the CSS and JS more complex and harder to reason about. I'd rather look to bigger potential wins first - like deduplicating the tooltips subdocuments.

There's also an argument from consistency: We already put appropriate classes on the <details> tags we've added so far. It would be odd to economize on this one particular item type and not the others.

At any rate, I don't want to hold up the whole PR for the next week; I'm find landing this piece as-is and we can discuss more when I'm back.

@jsha
Copy link
Contributor

jsha commented May 2, 2021 via email

@bors
Copy link
Contributor

bors commented May 2, 2021

📌 Commit 0d52eb9 has been approved by jsha

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels May 2, 2021
@bors
Copy link
Contributor

bors commented May 2, 2021

⌛ Testing commit 0d52eb9 with merge 6b5de7a...

@bors
Copy link
Contributor

bors commented May 2, 2021

☀️ Test successful - checks-actions
Approved by: jsha
Pushing 6b5de7a to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label May 2, 2021
@bors bors merged commit 6b5de7a into rust-lang:master May 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rustdoc-js Area: Rustdoc's JS front-end A-rustdoc-ui Area: Rustdoc UI (generated HTML) merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants