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

Add background-color on clickable definitions in source code #88111

Merged
merged 1 commit into from
Aug 19, 2021

Conversation

GuillaumeGomez
Copy link
Member

Someone suggested to add a decoration on clickable elements in the source code pages:

Screenshot from 2021-08-17 14-49-39
Screenshot from 2021-08-17 14-49-47
Screenshot from 2021-08-17 14-49-52

The idea is to not disturb the reading while telling the reader "you can click on this one", which is why it's not a text decoration.

What do you think @rust-lang/rustdoc ?

r? @Nemo157

@GuillaumeGomez GuillaumeGomez added T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. A-rustdoc-ui Area: Rustdoc UI (generated HTML) labels Aug 17, 2021
@rust-highfive
Copy link
Collaborator

Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 17, 2021
Copy link
Member

@jhpratt jhpratt left a comment

Choose a reason for hiding this comment

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

LGTM with this fix.

src/librustdoc/html/static/css/themes/ayu.css Outdated Show resolved Hide resolved
@Manishearth
Copy link
Member

Not sure of the exact choice of colors but overall I think this is a good idea. @jsha ?

@CraftSpider
Copy link
Contributor

I like the idea, I think the middle theme looks okay, top and bottom themes should probably make the colors stand out a bit more, it's hard to tell which parts are the links at a glance.

@GuillaumeGomez
Copy link
Member Author

I actually found the middle standing out too much. 😆

@CraftSpider
Copy link
Contributor

That's fair I guess, I just worry that if they don't stand out enough they won't really help indicate which ones are links. My primary skills aren't in visual design though, I'll leave that decision to those who know more :P

@jhpratt
Copy link
Member

jhpratt commented Aug 17, 2021

I've already discussed this with Guillaume on Zulip…be happy that there's something happening, because what I proposed was along the lines of the middle and he didn't care for it 😛

@GuillaumeGomez
Copy link
Member Author

@Manishearth @jsha won't be back until October, since this is an improvement, even though maybe not perfect, I think it's fine to merge now. I'll add this to my "list of things to discuss when Jacob is back" haha.

Thanks everyone for the comments!

@bors: r=jhpratt

@bors
Copy link
Contributor

bors commented Aug 17, 2021

📌 Commit 7ce7fe7 has been approved by jhpratt

@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 Aug 17, 2021
@camelid
Copy link
Member

camelid commented Aug 17, 2021

I actually found the middle standing out too much. 😆

I like the idea overall, though I also feel that the Ayu stands out too much. Would that one look different with #88111 (comment) applied?

We could also consider styling it like Haskell's equivalent to Rustdoc, Haddock. They use a faint underline, and then add background color when the link is hovered:

image

image

I like how it's very subtle in Haddock, so it doesn't affect the reading experience, but it's strong enough to think, "Hey, I bet I can click that link!"

IIUC, nearly all of the identifiers should be clickable in the generated source view, so we just want to give a hint that the links are clickable without distracting from the reading view.

@GuillaumeGomez
Copy link
Member Author

I like the idea overall, though I also feel that the Ayu stands out too much. Would that one look different with #88111 (comment) applied?

No, I badly copied into the code, I modified using the web tools in the browser.

As for your suggestion, I also like it. Hummm... Damn, don't know what to do...

@Manishearth
Copy link
Member

@GuillaumeGomez seems fair to me!

@jhpratt
Copy link
Member

jhpratt commented Aug 17, 2021

Yeah, I'm not saying it couldn't be better, but imo something is better than nothing as a stopgap. Having it be non-intrusive is the issue.

@GuillaumeGomez
Copy link
Member Author

@camelid Do you mind opening an issue with your suggestion please?

@camelid
Copy link
Member

camelid commented Aug 17, 2021

@camelid Do you mind opening an issue with your suggestion please?

@GuillaumeGomez done: #88120.

bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 19, 2021
…laumeGomez

Rollup of 10 pull requests

Successful merges:

 - rust-lang#87818 (Fix anchors display in rustdoc)
 - rust-lang#87983 (Use more accurate spans when proposing adding lifetime to item)
 - rust-lang#88012 (Change WASI's `RawFd` from `u32` to `c_int` (`i32`).)
 - rust-lang#88031 (Make `BuildHasher` object safe)
 - rust-lang#88036 (Fix dead code warning when inline const is used in pattern)
 - rust-lang#88082 (Take into account jobs number for rustdoc GUI tests)
 - rust-lang#88109 (Fix environment variable getter docs)
 - rust-lang#88111 (Add background-color on clickable definitions in source code)
 - rust-lang#88129 (Fix dataflow graphviz bug, make dataflow graphviz modules public)
 - rust-lang#88136 (Move private_unused.rs test to impl-trait)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 75c6a91 into rust-lang:master Aug 19, 2021
@rustbot rustbot added this to the 1.56.0 milestone Aug 19, 2021
@GuillaumeGomez GuillaumeGomez deleted the background-color-jump-to-def branch August 19, 2021 08:15
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Sep 13, 2021
…und, r=camelid

Fix jump def background

Fixes rust-lang#88870.

I somehow badly wrote the color in rust-lang#88111.

r? `@camelid`
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Sep 13, 2021
…und, r=camelid

Fix jump def background

Fixes rust-lang#88870.

I somehow badly wrote the color in rust-lang#88111.

r? ``@camelid``
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Sep 14, 2021
…und, r=camelid

Fix jump def background

Fixes rust-lang#88870.

I somehow badly wrote the color in rust-lang#88111.

r? `@camelid`
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Sep 14, 2021
…und, r=camelid

Fix jump def background

Fixes rust-lang#88870.

I somehow badly wrote the color in rust-lang#88111.

r? ``@camelid``
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rustdoc-ui Area: Rustdoc UI (generated HTML) 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.

9 participants