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

rustdoc: use smaller number of colors to distinguish items #91480

Merged
merged 3 commits into from
Jan 1, 2022

Conversation

jsha
Copy link
Contributor

@jsha jsha commented Dec 3, 2021

This reduces visual distractions when reading method signatures.

As discussed in #59845 (comment), this categorizes items into one of six colors (down from thirteen):

  • method, function (ochre #AD7C37)
  • trait, trait alias (dark slate blue #6E4FC9)
  • enum, struct, type alias, union, primitive (maroon #AD378A)
  • static, module, keyword, associated type, foreign type (steel blue #3873AD)
  • macro (green #068000)
  • generic params, self, Self (unmarked black #000000)

I slightly tweaked the actual color values so they'd have the same lightness (previously the trait color stood out much more than the others). And I made the color for links in general consistently use steel blue (previously there was a slightly different color for "search-failed").

The ayu and dark themes have been updated according to the same logic. I haven't changed any of the color values in those themes, just their assignment to types.

Demo:

https://rustdoc.crud.net/jsha/fewer-colors/std/string/struct.String.html
https://rustdoc.crud.net/jsha/fewer-colors/std/vec/struct.Vec.html
https://rustdoc.crud.net/jsha/fewer-colors/std/io/trait.Read.html
https://rustdoc.crud.net/jsha/fewer-colors/std/iter/trait.Iterator.html

@rust-highfive
Copy link
Collaborator

Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez

@rust-highfive
Copy link
Collaborator

r? @ollie27

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 3, 2021
@PatchMixolydic
Copy link
Contributor

PatchMixolydic commented Dec 3, 2021

I feel that giving functions and macros the same colour might cause confusion when a macro has the same name as a function. Consider this image from the perspective of a new programmer who wants to find out more about the panic! macro:
Image of a rustdoc search showing that core::panic and core:: panicking::panic have the same colour
Of course, a sufficiently knowledgeable user might already know that core::panic is the macro, might know that (most) macros are located in the crate root, or might happen to read the description of core::panicking::panic, but this could cause more confusion if there was a function and a macro with the same name in the same module without such helpful documentation. Because of this, I think that either macros should retain their current green colouration (removing any redundancy with attribute/derive macros if necessary) or function-like macros should be displayed with an exclamation point following their paths (ie. core::panic! instead of core::panic).

@GuillaumeGomez
Copy link
Member

I agree with @PatchMixolydic. They're close enough that it would make it confusing.

Another thing I wonder is if we should group by "kind of types". For example (by groups):

  • const, type aliases and trait aliases
  • struct, enum and union
  • trait
  • function
  • macro

I'm not too sure where to put primitives. Currently it's the same color as struct, enum and union, I guess it's fine as is. What do you think?

@jyn514 jyn514 closed this Dec 3, 2021
@jyn514 jyn514 reopened this Dec 3, 2021
@jyn514 jyn514 closed this Dec 3, 2021
@jyn514 jyn514 reopened this Dec 3, 2021
@jyn514
Copy link
Member

jyn514 commented Dec 3, 2021

(ugh, I hate GitHub on mobile)

I think we should only have different colors for things that could be confused in the search page. Since the only things that can have the same name must be in different namespaces, that suggests that we should have one color per namespace (value namespace, type ns, macro ns).

I like the idea of doing this with the symbols you'd see in the source (e.g. !) but I don't think it works because that only helps for function-like macros, not values, types, or derive macros.

@jyn514 jyn514 added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Dec 3, 2021
@jsha
Copy link
Contributor Author

jsha commented Dec 3, 2021

Good point about macros, particularly that not all macros are function-like, so styling as functions doesn't necessarily make sense. I've restored macros to having their own color. It was nice having just four colors, but since the primary motivation here is to make method signatures less distracting, and macros don't contribute to that problem, I think that's okay.

I think we should only have different colors for things that could be confused in the search page.

I think we rely too much on colors in the search page to distinguish types. If distinguishing types is important (and I think it is), we should put the type as text in the search results. That way it's accessible to colorblind people, and also understandable to people who haven't figured out what the colors mean (and it helps learn what the colors mean). But that's a change for another day. :-)

Since the only things that can have the same name must be in different namespaces, that suggests that we should have one color per namespace (value namespace, type ns, macro ns).

I like in theory the idea of mapping color distinctions onto a language concept (namespaces). This PR actually is pretty close to that. One difference: We still distinguish traits vs (enums, structs, unions), even though traits are also in the type namespace. I think the distinction between traits and other types is valuable and I'd like to keep it. Is there a specific change you'd like to make so colors align better with namespaces?

@jsha
Copy link
Contributor Author

jsha commented Dec 3, 2021

(Updated the branch and the demo)

@camelid
Copy link
Member

camelid commented Dec 3, 2021

When making a large UI change like this, please open a thread on Zulip so the whole team can discuss it :)

@camelid camelid changed the title rustdoc: use smaller number of colors to distinguish types rustdoc: use smaller number of colors to distinguish items Dec 3, 2021
@GuillaumeGomez
Copy link
Member

Looks good to me as is. Thanks for working on this!

I think we rely too much on colors in the search page to distinguish types. If distinguishing types is important (and I think it is), we should put the type as text in the search results. That way it's accessible to colorblind people, and also understandable to people who haven't figured out what the colors mean (and it helps learn what the colors mean). But that's a change for another day. :-)

This is actually a very good point!

When making a large UI change like this, please open a thread on Zulip so the whole team can discuss it :)

You're absolutely right, let's hear from others as well.

cc @rust-lang/rustdoc

@camelid
Copy link
Member

camelid commented Dec 3, 2021

jsha and I discussed the trait color on Zulip. I tweaked the trait color to match the "lightness" (see the Zulip thread for details on what that is) of the type color, and I think it looks more similar to the other colors now.

The current plan is this:

  • hyperlinks: #3873AD
  • value namespace: #AD7C37
  • type namespace: #AD378A
  • trait namespace: #6E4FC9

(Note that the hyperlink color is not being changed AFAICT.)

@jsha
Copy link
Contributor Author

jsha commented Dec 3, 2021

After some discussion on Zulip I've updated the branch and the demo to lighten the trait color from #5137AD to #6E4FC9. You can see this as a palette at https://color.adobe.com/rustdoc%20proposed%20theme-color-theme-19077288. If you want to play around with it, click "Create using Theme".

@camelid camelid assigned GuillaumeGomez and unassigned ollie27 Dec 3, 2021
@jyn514
Copy link
Member

jyn514 commented Dec 3, 2021

@camelid just to clarify - are macros using different colors for function-like and derive macros? Or is there only one color for the macro namespace? I don't know what the current behavior is.

(Either seems fine to me, but seems good to clarify)

@@ -11,7 +11,7 @@ reload:

assert-css: ("#toggle-all-docs", {"color": "rgb(0, 0, 0)"})
assert-css: (".fqn .in-band a:nth-of-type(1)", {"color": "rgb(0, 0, 0)"})
assert-css: (".fqn .in-band a:nth-of-type(2)", {"color": "rgb(173, 68, 142)"})
assert-css: (".fqn .in-band a:nth-of-type(2)", {"color": "rgb(173, 55, 138)"})
Copy link
Member

Choose a reason for hiding this comment

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

Yikes, it's worrying that only one test had to change because of this massive color change ;)

Copy link
Member

Choose a reason for hiding this comment

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

I think more tests will be added. :)

@jsha
Copy link
Contributor Author

jsha commented Dec 3, 2021

There's just one color for all types of macros.

@camelid
Copy link
Member

camelid commented Dec 3, 2021

@camelid just to clarify - are macros using different colors for function-like and derive macros? Or is there only one color for the macro namespace? I don't know what the current behavior is.

(Either seems fine to me, but seems good to clarify)

That's a good point; macros aren't in the color palette IIRC. @jsha could you put the macro color in the palette instead of the unstable item-info color?

@jsha
Copy link
Contributor Author

jsha commented Dec 3, 2021

could you put the macro color in the palette instead of the unstable item-info color?

already did, and updated the link above :-) https://color.adobe.com/rustdoc%20proposed%20theme-color-theme-19077288

This reduces visual distractions when reading method signatures.
@jsha jsha added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Dec 10, 2021
@rust-log-analyzer

This comment has been minimized.

@jsha
Copy link
Contributor Author

jsha commented Dec 11, 2021

I pushed a fix for an issue where associated types were incorrectly getting generated with class="type" (which is meant for typedefs).

I'm happy to write some tests for colors, but it's pretty error prone with browser-ui test as it is today. I filed GuillaumeGomez/browser-UI-test#227 for an update to make it easier to specify expected colors in the same way we specify them in the CSS (hex literals instead of rgba tuples).

@rust-log-analyzer

This comment has been minimized.

@GuillaumeGomez
Copy link
Member

Looks good to me! I'll wait for the others who commented to take a look first but then r=me.

@jsha
Copy link
Contributor Author

jsha commented Dec 30, 2021

@GuillaumeGomez said:

I'll wait for the others who commented to take a look first but then r=me.

So far we have an approval from @jyn514. @camelid and @PatchMixolydic you were the others who commented. I believe I've satisfactorily addressed all your comments. Good to go?

@camelid
Copy link
Member

camelid commented Dec 30, 2021

I haven't looked at this in a while, but I think it's fine to land this and make tweaks later.

@PatchMixolydic
Copy link
Contributor

Looks good to me 👍

@jsha
Copy link
Contributor Author

jsha commented Dec 31, 2021

@bors r=GuillaumeGomez rollup

@bors
Copy link
Contributor

bors commented Dec 31, 2021

📌 Commit 3f517fb has been approved by GuillaumeGomez

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Dec 31, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 31, 2021
…askrgr

Rollup of 7 pull requests

Successful merges:

 - rust-lang#90383 (Extend check for UnsafeCell in consts to cover unions)
 - rust-lang#91375 (config.rs: Add support for a per-target default_linker option.)
 - rust-lang#91480 (rustdoc: use smaller number of colors to distinguish items)
 - rust-lang#92338 (Add try_reserve and  try_reserve_exact for OsString)
 - rust-lang#92405 (Add a couple needs-asm-support headers to tests)
 - rust-lang#92435 (Sync rustc_codegen_cranelift)
 - rust-lang#92440 (Fix mobile toggles position)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 72e36d4 into rust-lang:master Jan 1, 2022
@rustbot rustbot added this to the 1.59.0 milestone Jan 1, 2022
@jsha jsha deleted the fewer-colors branch January 2, 2022 16:29
@aloucks
Copy link
Contributor

aloucks commented Feb 7, 2022

What is the formal process for requesting this to be all or partially reverted? The PR was a step backwards and universally disliked.

Merging the color options makes things harder to find and understand. I've been looking for green text to identify enums in method signatures and now it's impossible. PR #92660 makes things even worse by combining them all into a single section.

Having distinct colors and sections improves visibility and discovery. The first analogy that comes to mind are phone numbers.

867-5309 is much easier to visually process than 8675309.

@GuillaumeGomez
Copy link
Member

@Ten0
Copy link

Ten0 commented Oct 5, 2022

For anyone interested this has bothered me enough that I've created and been testing for a few weeks the following userscript to bring back colors to my rustdoc browsing:
https://gist.github.com/Ten0/564a81a64f9316db4051a4f2afe2b484

(You may use it via Tampermonkey on any major browser)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.