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

De-emphasize unstable items in rustdoc item table #118226

Closed
wants to merge 1 commit into from

Conversation

dtolnay
Copy link
Member

@dtolnay dtolnay commented Nov 24, 2023

For many years, since 0a46933 unstable APIs used to be shown in a faded color (both the item name and the description). See https://doc.rust-lang.org/1.54.0/std/marker/index.html.

In #85651 the description became un-faded. See https://doc.rust-lang.org/1.55.0/std/marker/index.html.

In #107336 the item name became un-faded too. Compare https://doc.rust-lang.org/1.68.0/std/marker/index.html vs https://doc.rust-lang.org/1.69.0/std/marker/index.html.

It is difficult for me to tell whether either of these changes was intentional. None of the PR descriptions or commit messages even mentions stability at all.

This PR restores fading the unstable item name. I find this visual separation to be helpful in getting oriented about the sort order of the items.

Before:

Screenshot from 2023-11-23 18-53-18

After:

Screenshot from 2023-11-23 18-53-24

(the screenshots also include my other PR #118224.)

@rustbot
Copy link
Collaborator

rustbot commented Nov 24, 2023

r? @GuillaumeGomez

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

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Nov 24, 2023
@rustbot
Copy link
Collaborator

rustbot commented Nov 24, 2023

Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez, @jsha

Comment on lines 633 to 634
item.stability(tcx).as_ref().map(|s| s.is_unstable() && s.feature != sym::rustc_private)
== Some(true)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
item.stability(tcx).as_ref().map(|s| s.is_unstable() && s.feature != sym::rustc_private)
== Some(true)
item.stability(tcx).as_ref().is_some_and(|s| s.is_unstable() && s.feature != sym::rustc_private)

@Urgau
Copy link
Member

Urgau commented Nov 24, 2023

I would like to remark that the current color (text: #A07DD3 and bg: #353535) only have a contrast of 3.72 which is below the recommended 4.5 minimum.

Adding opacity to them (text: #9072B9) makes to drop to 3.09, even below.

It would be better to use another color rather the adjusting the opacity, but it's a bigger change.

@GuillaumeGomez
Copy link
Member

In any case, the experimental items are marked as such. Not sure the color change is really improving anything here.

@bors
Copy link
Contributor

bors commented Nov 24, 2023

☔ The latest upstream changes (presumably #118232) made this pull request unmergeable. Please resolve the merge conflicts.

@jsha
Copy link
Contributor

jsha commented Nov 24, 2023

As far as I can tell from those PR descriptions, the un-fading was unintentional.

Coincidentally, I had been meaning to propose un-fading these for accessibility and clarity reasons. Though I agree that if the un-fading was intentional the right thing to do is to revert it, then discuss making the change intentionally.

For context here's my thinking about fading: As @Urgau points out, it reduces the contrast below recommended accessibility standards. Also it's very implicit and hard to figure out on its own.

In the first screenshot, there are three different ways that the unstable items are marked:

  • Sort order
  • Experimental tag
  • fading

We only need one of these. We should avoid fading for the reasons above. We should avoid sort order because it's implicit, not explicit. IMO the Experimental tag is sufficient. But if we decide we want unstable items listed separately from other items, we shouldn't do it with sort order (implicit), we should do it as a separate section (explicit). For instance:

Traits

Copy Types whose values can be duplicated simply by copying bits.
Send Types that can be transferred across thread boundaries.
Sized Types with a constant size known at compile time.
Sync Types for which it is safe to share references between threads.
Unpin Types that can be safely moved after being pinned.

Traits (Experimental)

Destruct A marker for types that can be dropped.
DiscriminantKind Compiler-internal trait used to indicate the type of enum discriminants.
PointerLike A marker for pointer-like types.
StructuralEq Required trait for constants used in pattern matches.
StructuralPartialEq Required trait for constants used in pattern matches.
Tuple A marker for tuple types.
Unsize Types that can be “unsized” to a dynamically-sized type.

fmease added a commit to fmease/rust that referenced this pull request Nov 25, 2023
Replace `option.map(cond) == Some(true)` with `option.is_some_and(cond)`

Requested by `@fmease` in rust-lang#118226 (review).

There is also a much larger number of `option.map_or(false, cond)` that can be changed separately if someone wants.

r? fmease
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Nov 25, 2023
Rollup merge of rust-lang#118253 - dtolnay:issomeand, r=compiler-errors

Replace `option.map(cond) == Some(true)` with `option.is_some_and(cond)`

Requested by `@fmease` in rust-lang#118226 (review).

There is also a much larger number of `option.map_or(false, cond)` that can be changed separately if someone wants.

r? fmease
@jdahlstrom
Copy link

jdahlstrom commented Nov 26, 2023

IMO the Experimental tag is sufficient.

FWIW I strongly disagree. Nightly features shouldn't be in the stable documentation in the first place – because they're not just experimental, they're literally not available! – and if they have to be because reasons, they should at least be as far separated and de-emphasized from actual available items as possible. They're simply visual noise that makes it more difficult to skim and scan the docs and find what you want – and even worse, in the sidebar they're not distinguished at all! Particularly It gives an impression that nightly Rust is the "preferred" version to use, even after eight years since 1.0. (Why isn't there even "Auto-hide experimental items" in the settings dropdown?)

But if we decide we want unstable items listed separately from other items [...] we should do it as a separate section

This would be better than nothing indeed.

@GuillaumeGomez
Copy link
Member

(Why isn't there even "Auto-hide experimental items" in the settings dropdown?)

That's actually a pretty good question! I think I'd prefer this solution instead.

@notriddle
Copy link
Contributor

That's actually a pretty good question! I think I'd prefer this solution instead.

I'd prefer not to do that. It seems too much like treating configuration options as a crutch.

If they're only available on nightly toolchains, then only nightly toolchains should have them in the docs. Additionally, since many people use nightly toolchains specifically to access experimental features, they shouldn't be de-emphasized when they appear in nightly docs.

@dtolnay
Copy link
Member Author

dtolnay commented Jan 21, 2024

It sounds like there are multiple compelling reasons not to use opacity like this, even if originally removing it was unintentional.

Thanks for the review!

@dtolnay dtolnay closed this Jan 21, 2024
@dtolnay dtolnay deleted the fade branch January 21, 2024 04:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. 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