Highlight crate links like normal links by camelid · Pull Request #75842 · rust-lang/rust

camelid · 2020-08-23T17:08:33Z

Fixes#75823.

Cc @jyn514

rust-highfive · 2020-08-23T17:08:36Z

r? @ollie27

(rust_highfive has picked a reviewer for you, use r? to override)

camelid · 2020-08-23T17:08:52Z

@rustbot modify labels: T-rustdoc

camelid · 2020-08-23T17:11:09Z

I'm waiting on a rustc build to see if this works - for some reason it's rebuilding LLVM... :(

camelid · 2020-08-23T17:24:37Z

Fixed formatting

camelid · 2020-08-23T17:25:31Z

- format!("<li><a href=\"{}index.html\">{}</li>", ensure_trailing_slash(s), s)
+ format!(
+ "<li><a class=\"mod\" href=\"{}index.html\">{}</a></li>",
+ ensure_trailing_slash(s),
+ s
+ )


Using class mod should probably be temporary (I should probably be a separate class crate). However, the ul that this is in (starting on line 1065) already has class mod!

I added a missing closing </a> tag also.

In the other link class="mod" was also added.

camelid · 2020-08-23T17:54:29Z

r? @jyn514

jyn514 · 2020-08-23T17:56:42Z

I think ollie might actually have been a better reviewer, I don't know this part of the code very well ... let's

r? @GuillaumeGomez

since he has so much free time ;)

GuillaumeGomez · 2020-08-23T19:54:28Z

Looks good to me. Is there anything else to be done here?

since he has so much free time ;)

😝

camelid · 2020-08-23T20:09:36Z

I think I should add a new CSS class crate instead of using mod.

camelid · 2020-08-23T20:29:29Z

Actually, since we want the styles to be the same, it might be better to not add a separate class since it might get out of date if the module styles are updated.

camelid · 2020-08-23T20:31:05Z

I wasn't able to verify that the styling looks correct since I've been having some issues building the compiler (https://rust-lang.zulipchat.com/#narrow/stream/182449-t-compiler.2Fhelp/topic/ICE.20when.20compiling).

camelid · 2020-08-23T20:33:50Z

One final thing which is somewhat unrelated is that the title, description, and heading for the index page are all different:

rust/src/librustdoc/html/render/mod.rs

Lines 1047 to 1057 in d02a209

let page = layout::Page{
title:"Index of crates",
css_class:"mod",
root_path:"./",
static_root_path: cx.shared.static_root_path.as_deref(),
description:"List of crates",
keywords:BASIC_KEYWORDS,
resource_suffix:&cx.shared.resource_suffix,
extra_scripts:&[],
static_extra_scripts:&[],
};

It might be good to consolidate them, but that could be done in a separate PR.

pickfire · 2020-08-24T01:13:31Z

 .map(|s| {
- format!("<li><a href=\"{}index.html\">{}</li>", ensure_trailing_slash(s), s)
+ format!(
+ "<li><a class=\"mod\" href=\"{}index.html\">{}</a></li>",


Suggested change
"<li><a class=\"mod\" href=\"{}index.html\">{}</a></li>",
"<li><a class='mod' href='{}index.html'>{}</a></li>",

We're using double quotes everywhere else, so for consistency, I think we should keep them.

I saw that we also use single quotes for quite a lot of place too. From what I see, maybe more than double quotes.

179 single quotes and 230 double quotes (looking for =' and =\"). I wonder how we ended up in this mess... We should clean that up.

Why not just make all single quotes? We don't need to escape that way..

Had some cases where I saw:
<div onclick='func('clicked');'>...</div>
So mostly irrelevant here but still afraid me. Even more considering that for now, the generated UI isn't tested.

We could change that to
<div onclick="func('clicked');">...</div>

Or to:
<div onclick='func("clicked");'>...</div>
😛

camelid · 2020-08-26T20:56:34Z

@GuillaumeGomez I think this PR is ready. Do you want to merge? @pickfire or someone else could do a follow-up PR to make the quote character consistent in this file if that's what's decided upon.

jyn514 · 2020-08-26T21:06:23Z

I think this PR is ready.
I'm waiting on a rustc build to see if this works - for some reason it's rebuilding LLVM... :(

Can you post a screenshot of what this now looks like?

GuillaumeGomez · 2020-08-26T21:43:12Z

Yes, just a screenshot before/after and then I think we're good to go!

camelid · 2020-08-26T21:45:05Z

Still waiting on my build :)

The before is at https://doc.rust-lang.org/nightly/nightly-rustc:

camelid · 2020-08-26T22:56:12Z

Wow, my build is taking forever. I just stopped and started it in case that’s the issue. ./x.py doc --stage 1 is the command to use, right?

jyn514 · 2020-08-26T23:01:59Z

Wow, my build is taking forever. I just stopped and started it in case that’s the issue. ./x.py doc --stage 1 is the command to use, right?

Yes, that sounds right. Try running with -j1 if you aren't already, you might be thrashing memory to disk and back. It takes me about an hour on my laptop with -j1.

camelid · 2020-08-26T23:28:38Z

I have several gigabytes of free memory though, so would that still help?

jyn514 · 2020-08-27T01:08:18Z

Looks good to me!

@bors r+

bors · 2020-08-27T01:08:19Z

📌 Commit 4419e04 has been approved by jyn514

camelid · 2020-08-27T01:09:04Z

Never could get my build to finish :(

bors · 2020-08-27T02:51:07Z

⌛ Testing commit 4419e04 with merge 18b0585...

bors · 2020-08-27T05:29:35Z

☀️ Test successful - checks-actions, checks-azure
Approved by: jyn514
Pushing 18b0585 to master...

GuillaumeGomez · 2020-08-27T08:57:55Z

@camelid In case you want to retry to build doc locally: x.py doc library/std --stage 1 --jobs 2 (--jobs 2 is just to prevent your computer to burn too much haha). Then it's generated in build/[your arch]/doc/std/index.html.

camelid · 2020-08-30T00:54:44Z

@GuillaumeGomez Thanks, I'll try that!

camelid · 2020-08-30T01:45:08Z

The build completed successfully, but that index page is not the crate index, so it is not affected by this PR. I think I have to document the compiler to get the right page.

jyn514 · 2020-08-30T01:49:10Z

@camelid try compiler-docs = true in config.toml

camelid · 2020-08-30T03:11:43Z

@jyn514 I'm curious: is there a reason that is not the default?

jyn514 · 2020-08-30T03:16:39Z

Not sure, sorry. You could ask on zulip.

My guess though is that most of the compiler docs are maintained out of tree at rustc-dev-guide.rust-lang.org so people don't look at the API docs as much? I use the online ones a lot though.

Use double quote for rustdoc html r? `@GuillaumeGomez` Feels scary without escaping stuff when I looked at the code, probably susceptible to XSS. Follow up of rust-lang#75842

rust-highfive assigned ollie27 Aug 23, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 23, 2020

rustbot added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Aug 23, 2020

camelid commented Aug 23, 2020
View reviewed changes

Comment threadsrc/librustdoc/html/render/mod.rs Outdated

Highlight crate links like normal links
4419e04

camelid force-pushed the highlight-crate-links branch from 85482a6 to 4419e04Compare August 23, 2020 17:24

camelid commented Aug 23, 2020
View reviewed changes

rust-highfive assigned jyn514 and unassigned ollie27 Aug 23, 2020

rust-highfive assigned GuillaumeGomez and unassigned jyn514 Aug 23, 2020

GuillaumeGomez approved these changes Aug 23, 2020
View reviewed changes

camelid marked this pull request as ready for review August 23, 2020 20:29

camelid mentioned this pull request Aug 23, 2020
ICE: ./x.py doc --stage 1 panics #75849
Closed

pickfire reviewed Aug 24, 2020
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 Aug 27, 2020

bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 27, 2020

bors merged commit 18b0585 into rust-lang:masterAug 27, 2020

camelid deleted the highlight-crate-links branch August 30, 2020 00:54

pickfire mentioned this pull request Aug 30, 2020
Use double quote for rustdoc html #76096
Merged

cuviper added this to the 1.48.0 milestone Nov 17, 2023

	"<li><a class=\"mod\" href=\"{}index.html\">{}</a></li>",
	"<li><a class='mod' href='{}index.html'>{}</a></li>",

Uh oh!

Uh oh!

Conversation

camelid commented Aug 23, 2020

Uh oh!

rust-highfive commented Aug 23, 2020

Uh oh!

camelid commented Aug 23, 2020

Uh oh!

Uh oh!

camelid commented Aug 23, 2020

Uh oh!

camelid commented Aug 23, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

camelid commented Aug 23, 2020

Uh oh!

jyn514 commented Aug 23, 2020

Uh oh!

GuillaumeGomez commented Aug 23, 2020

Uh oh!

camelid commented Aug 23, 2020

Uh oh!

camelid commented Aug 23, 2020

Uh oh!

camelid commented Aug 23, 2020

Uh oh!

camelid commented Aug 23, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pickfireAug 24, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

GuillaumeGomezAug 24, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

camelid commented Aug 26, 2020

Uh oh!

jyn514 commented Aug 26, 2020

Uh oh!

GuillaumeGomez commented Aug 26, 2020

Uh oh!

camelid commented Aug 26, 2020

Uh oh!

camelid commented Aug 26, 2020

Uh oh!

jyn514 commented Aug 26, 2020

Uh oh!

camelid commented Aug 26, 2020

Uh oh!

jyn514 commented Aug 27, 2020

Uh oh!

bors commented Aug 27, 2020

Uh oh!

camelid commented Aug 27, 2020

Uh oh!

bors commented Aug 27, 2020

Uh oh!

bors commented Aug 27, 2020

Uh oh!

GuillaumeGomez commented Aug 27, 2020

camelid commented Aug 23, 2020•
edited
Loading

pickfireAug 24, 2020•
edited
Loading

GuillaumeGomezAug 24, 2020•
edited
Loading