Make source links look cleaner #92602

jsha · 2022-01-06T01:21:13Z

Change from syntaxy-looking [src] to the plain word "source".

Part of #59851

r? @GuillaumeGomez

Demo: https://rustdoc.crud.net/jsha/source-link-2/std/string/struct.String.html

Discussed on Zulip.

rust-highfive · 2022-01-06T01:21:15Z

Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez,@Folyd

camelid · 2022-01-06T01:57:32Z

Looks good! I don't see the "stable since" change though? Also, the color of "source" looks a little too pale in the light theme.

jsha · 2022-01-06T02:06:29Z

I don't see the "stable since" change though?

My original version added the "stable since" language but on reviewing the Zulip thread I saw there wasn't really consensus on that, and it's not the main point of the PR, so I reverted it (but forgot to update the PR description). I've now updated the PR description.

Also, the color of "source" looks a little too pale in the light theme.

I'll bump it up a little.

camelid · 2022-01-06T03:54:29Z

That looks better, although I think the links should be still deeper in color. The current color in some ways feels more distracting than if it were darker because it's on the edge of being less visible vs more visible. It just doesn't look right to me for whatever reason.

jsha · 2022-01-06T04:07:29Z

Just pushed a version where they get normal opacity like any other link. Lemme know what you think (remember to hard-reload if needed).

camelid · 2022-01-06T05:15:58Z

That looks better, thanks.

src/librustdoc/html/render/mod.rs

GuillaumeGomez · 2022-01-06T13:45:25Z

Apart from my comment, looks good to me.

GuillaumeGomez · 2022-01-06T15:28:15Z

So looks good to me. I see some 👎 signs on the first comment.

@euclio @falaosu: What don't you like with this approach?

euclio · 2022-01-06T17:01:47Z

My original objection was that the new text isn't visually distinct enough, though after reading the linked issue I see that there's a justification for this.

That said, I don't think that this does a good enough job of communicating that these are clickable actions. The brackets in the current style do a good job of making it clear that these are buttons, and "-" communicates the same amount of information as "collapse" in fewer characters. For such commonly-used actions that are repeated across the page, we can use symbols to communicate these actions effectively, and that reduces overall clutter.

"source" is maybe fine as a link visually, since it actually takes you to a separate page (though there's no underline on hover). I don't think collapse should just be a link, though, since it only performs an action on the current page. Assuming that we want to change the other "[-]"s on the page for consistency, I would expect a symbol like "▶︎" for each individual section, and "▼ collapse all" at the top.

jsha · 2022-01-06T17:09:02Z

"source" is maybe fine as a link visually, since it actually takes you to a separate page (though there's no underline on hover).

I'll add underline on hover.

I don't think collapse should just be a link, though, since it only performs an action on the current page. Assuming that we want to change the other "[-]"s on the page for consistency, I would expect a symbol like "▶︎" for each individual section, and "▼ collapse all" at the top.

I changed [-] to collapse because 1.0.0 · [-] · source looked wrong. But I could make it 1.0.0 · source · [-], in the spirit of "change one thing at a time."

GuillaumeGomez · 2022-01-06T18:48:43Z

"source" is maybe fine as a link visually, since it actually takes you to a separate page (though there's no underline on hover). I don't think collapse should just be a link, though, since it only performs an action on the current page. Assuming that we want to change the other "[-]"s on the page for consistency, I would expect a symbol like "▶︎" for each individual section, and "▼ collapse all" at the top.

To be fair, this button at the top of the page is a bit on its own since its behaviour is different than the other collapse toggles. But at least I see your point.

@jsha: Just realized that you didn't add a test for the text of the top collapse button (to check it's "expand"/"collapse" when it should). Could you add one as well please? And since you said you would add underline on hover, can you add a test for this as well please?

jsha · 2022-01-06T20:08:33Z

Alright, I've pushed new code reverting the "collapse" text back to [-]. Updated the demo.

Just realized that you didn't add a test for the text of the top collapse button (to check it's "expand"/"collapse" when it should).

Since this is reverted, no behavior here has changed. I'll look at adding a test for the + / - change though.

And since you said you would add underline on hover, can you add a test for this as well please?

Will do.

camelid · 2022-01-06T21:01:24Z

FWIW, I liked the [-] -> collapse change. But, in the interest of incremental progress, I think it's fine to discuss that later.

GuillaumeGomez · 2022-01-06T22:24:20Z

I liked it as well, maybe for another time...

GuillaumeGomez · 2022-01-07T20:50:42Z

Thanks for the tests! Just one last question and it's good for me. :)

GuillaumeGomez · 2022-01-08T14:37:44Z

src/librustdoc/html/render/mod.rs

@@ -315,8 +315,8 @@ impl AllTypes {
                 <span class=\"out-of-band\">\
                     <span id=\"render-detail\">\
                         <a id=\"toggle-all-docs\" href=\"javascript:void(0)\" \
-                            title=\"collapse all docs\">\
-                             [<span class=\"inner\">&#x2212;</span>]\
+                             title=\"collapse all docs\">\


Why these changes?

GuillaumeGomez · 2022-01-08T14:41:33Z

When I hover the source links, they don't get underlined. Could fix it and add a test for it too please?

Change from syntaxy-looking [src] to the plain word "source".

jsha · 2022-01-08T14:51:21Z

When I hover the source links, they don't get underlined. Could fix it and add a test for it too please?

The source link at the top of the page is part of the main-heading, where links get underline on hover.

The source link to the right of the code headings is part of those headings, which don't get underline on hover.

I think this PR is consistent with how we currently do things. If we want to add underline-on-hover everywhere, I'm in favor, but we shouldn't do it piecemeal.

I pushed to remove the spurious diffs. Thanks for catching!

GuillaumeGomez · 2022-01-08T14:57:51Z

It's fine as is then. I think it's worth discussing for a follow-up which links should be underlined or not but it's for later. r=me once CI pass and thanks!

jsha · 2022-01-09T18:04:22Z

@bors r+ rollup

bors · 2022-01-09T18:04:24Z

📌 Commit 962c0a4 has been approved by jsha

jsha · 2022-01-09T18:06:14Z

@bors r=GuillaumeGomez

bors · 2022-01-09T18:06:16Z

💡 This pull request was already approved, no need to approve it again.

There's another pull request that is currently being tested, blocking this pull request: Hash Ident spans in all HIR structures #92534

bors · 2022-01-09T18:06:17Z

📌 Commit 962c0a4 has been approved by GuillaumeGomez

…askrgr Rollup of 7 pull requests Successful merges: - rust-lang#92248 (Normalize struct tail type when checking Pointee trait) - rust-lang#92357 (Fix invalid removal of newlines from doc comments) - rust-lang#92602 (Make source links look cleaner) - rust-lang#92636 (Normalize generator-local types with unevaluated constants) - rust-lang#92693 (Release notes: add `Result::unwrap_{,err_}unchecked`) - rust-lang#92702 (Clean up lang_items::extract) - rust-lang#92717 (update miri) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

- Make "since" version numbers grey again (regressed in rust-lang#92602). - Remove unneeded selectors for when crate filter dropdown is a sibling of search-input. - Crate filter dropdown doesn't need to be 100% width on mobile. - Only build crate filter dropdown when there is more than one crate. - Remove unused addCrateDropdown.

Rustdoc style cleanups - Make "since" version numbers grey again (regressed in rust-lang#92602). - Remove unneeded selectors for when crate filter dropdown is a sibling of search-input. - Crate filter dropdown doesn't need to be 100% width on mobile. - Only build crate filter dropdown when there is more than one crate. - Remove unused addCrateDropdown Demo: https://rustdoc.crud.net/jsha/style-cleanups/std/string/struct.String.html r? `@GuillaumeGomez`

rust-highfive assigned GuillaumeGomez Jan 6, 2022

rustbot added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Jan 6, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 6, 2022

This comment has been minimized.

Sign in to view

jsha force-pushed the source-link-2 branch from e06bbb0 to 151a533 Compare January 6, 2022 02:33

jsha changed the title ~~Make source links less obtrusive~~ Make source links look cleaner Jan 6, 2022

This comment has been minimized.

Sign in to view

jsha force-pushed the source-link-2 branch from 151a533 to 145db6b Compare January 6, 2022 03:44

GuillaumeGomez reviewed Jan 6, 2022

View reviewed changes

src/librustdoc/html/render/mod.rs Outdated Show resolved Hide resolved

jsha force-pushed the source-link-2 branch from 40c3fb0 to ccbf759 Compare January 6, 2022 15:00

GuillaumeGomez approved these changes Jan 6, 2022

View reviewed changes

jsha force-pushed the source-link-2 branch from ccbf759 to 877c673 Compare January 6, 2022 20:06

jsha force-pushed the source-link-2 branch 3 times, most recently from 0679668 to 22ad738 Compare January 6, 2022 20:26

jsha force-pushed the source-link-2 branch from 22ad738 to 7361955 Compare January 7, 2022 17:56

jsha force-pushed the source-link-2 branch 2 times, most recently from 8e55935 to 6b444ee Compare January 7, 2022 22:06

GuillaumeGomez reviewed Jan 8, 2022

View reviewed changes

Make source links look cleaner

962c0a4

Change from syntaxy-looking [src] to the plain word "source".

jsha force-pushed the source-link-2 branch from 6b444ee to 962c0a4 Compare January 8, 2022 14:50

GuillaumeGomez approved these changes Jan 8, 2022

View reviewed changes

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 Jan 9, 2022

matthiaskrgr mentioned this pull request Jan 10, 2022

Rollup of 7 pull requests #92719

Merged

bors merged commit a4ac4fa into rust-lang:master Jan 10, 2022

rustbot added this to the 1.60.0 milestone Jan 10, 2022

jsha deleted the source-link-2 branch January 12, 2022 23:33

jsha mentioned this pull request Jan 12, 2022

Rustdoc style cleanups #92830

Merged

camelid mentioned this pull request Jan 25, 2022

Simplify and unify rustdoc sidebar styles #92692

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make source links look cleaner #92602

Make source links look cleaner #92602

jsha commented Jan 6, 2022 •

edited

Loading

rust-highfive commented Jan 6, 2022

This comment has been minimized.

camelid commented Jan 6, 2022

jsha commented Jan 6, 2022

This comment has been minimized.

camelid commented Jan 6, 2022

jsha commented Jan 6, 2022 •

edited

Loading

camelid commented Jan 6, 2022

GuillaumeGomez commented Jan 6, 2022

GuillaumeGomez commented Jan 6, 2022

euclio commented Jan 6, 2022

jsha commented Jan 6, 2022

GuillaumeGomez commented Jan 6, 2022

jsha commented Jan 6, 2022

camelid commented Jan 6, 2022

GuillaumeGomez commented Jan 6, 2022

GuillaumeGomez commented Jan 7, 2022

GuillaumeGomez Jan 8, 2022

GuillaumeGomez commented Jan 8, 2022

jsha commented Jan 8, 2022

GuillaumeGomez commented Jan 8, 2022

jsha commented Jan 9, 2022

bors commented Jan 9, 2022

jsha commented Jan 9, 2022

bors commented Jan 9, 2022

bors commented Jan 9, 2022

Make source links look cleaner #92602

Make source links look cleaner #92602

Conversation

jsha commented Jan 6, 2022 • edited Loading

rust-highfive commented Jan 6, 2022

This comment has been minimized.

camelid commented Jan 6, 2022

jsha commented Jan 6, 2022

This comment has been minimized.

camelid commented Jan 6, 2022

jsha commented Jan 6, 2022 • edited Loading

camelid commented Jan 6, 2022

GuillaumeGomez commented Jan 6, 2022

GuillaumeGomez commented Jan 6, 2022

euclio commented Jan 6, 2022

jsha commented Jan 6, 2022

GuillaumeGomez commented Jan 6, 2022

jsha commented Jan 6, 2022

camelid commented Jan 6, 2022

GuillaumeGomez commented Jan 6, 2022

GuillaumeGomez commented Jan 7, 2022

GuillaumeGomez Jan 8, 2022

Choose a reason for hiding this comment

GuillaumeGomez commented Jan 8, 2022

jsha commented Jan 8, 2022

GuillaumeGomez commented Jan 8, 2022

jsha commented Jan 9, 2022

bors commented Jan 9, 2022

jsha commented Jan 9, 2022

bors commented Jan 9, 2022

bors commented Jan 9, 2022

jsha commented Jan 6, 2022 •

edited

Loading

jsha commented Jan 6, 2022 •

edited

Loading