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: redesign [+]/[−] controls #113074

Closed
wants to merge 5 commits into from

Conversation

notriddle
Copy link
Contributor

@notriddle notriddle commented Jun 26, 2023

Fixes #60255

Preview

Web preview: http://notriddle.com/rustdoc-html-demo-6/plus-minus-december-2023/std/iter/trait.Iterator.html

Image preview (GitHub's downscaling makes it hard to see some of what I'm talking about, so you might want to open the image in a new tab):
image

Image preview of "toggle all" button:
image

Description

This is a continuation of #107658 and #59851, making the toggles consistent with other buttons, instead of looking like syntax.

The tactics to improve the look of these controls:

  • When the toggle is expanded, the minus sign remains as dark as before, but the border is lighter. The plus sign has a border that's the same color as the label (black, on the default theme). This makes expansion buttons more prominent, easier to tell apart from collapse buttons.
  • The plus sign is slightly taller than wide, also to make it easier to tell apart from the minus sign.
    • The use of crispEdges has to get a bit strategic to make this come out right. I've tested it on Firefox, Safari, and Chromium, but it's a bit hard to be sure it works right on all setups.
    • Does anybody know some trick to do crispEdges on only the X axis, while still allowing antialiasing on the Y?
  • The "toggle all" button is modeled after the Help and Settings buttons, with a matching border, width, and +/− label.

@rustbot
Copy link
Collaborator

rustbot commented Jun 26, 2023

r? @jsha

(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 Jun 26, 2023
@rustbot
Copy link
Collaborator

rustbot commented Jun 26, 2023

Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez, @Folyd, @jsha

Some changes occurred in GUI tests.

cc @GuillaumeGomez

@rust-log-analyzer

This comment has been minimized.

@GuillaumeGomez
Copy link
Member

I think it's a great idea! A few comments now:

I find that the "toggle all" button attracts too much attention:

image

It has the same "level" of importance as the other buttons (help and settings). Also, its style differs from the other toggles.

For the other buttons, the + and - don't have the same look:

image

I think the appearance of the - makes it difficult to see. The appearance of the + is right on point for me. Also, I think they should have the same appearance.

@notriddle
Copy link
Contributor Author

I find that the "toggle all" button attracts too much attention

That seems like it's just dark specifically being weird. The light and ayu themes give the toolbar buttons much less prominence.

light:
image

ayu:
image

Should I make the "toggle all" button see-through on dark theme, like this?

dark (proposal):
image

For the other buttons, the + and - don't have the same look

#107658 claimed that the toggle design made it too hard to tell them apart from each other, so the color change was aimed at making the + and − easier to distinguish.

Do you have any alternative suggestions?

@GuillaumeGomez
Copy link
Member

Should I make the "toggle all" button see-through on dark theme, like this?

Yep with background color of button being the same color as the "general" background, looks great (or just transparent, as you prefer).

#107658 claimed that the toggle design made it too hard to tell them apart from each other, so the color change was aimed at making the + and − easier to distinguish.

Do you have any alternative suggestions?

I don't agree that this is an issue, quite the opposite when I see the result. My attention is bugged by this difference. Looking at the sign tells me if it's for expanding or collapsing, I don't think adding something to differentiate +/- buttons is a good idea. But as usual, we can see what the rest of the team thinks about it. I personally think it makes it more distracting than actually useful.

@rustbot
Copy link
Collaborator

rustbot commented Jun 28, 2023

Some changes occurred in HTML/CSS themes.

cc @GuillaumeGomez

@notriddle
Copy link
Contributor Author

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@GuillaumeGomez
Copy link
Member

Looks good to me, thanks!

@notriddle
Copy link
Contributor Author

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented Jun 28, 2023

Team member @notriddle has proposed to merge this. The next step is review by the rest of the tagged team members:

Concerns:

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Jun 28, 2023
@GuillaumeGomez
Copy link
Member

Don't forget to fix the CI failure. ;)

@Nemo157
Copy link
Member

Nemo157 commented Jun 28, 2023

One tiny issue I see is that this makes the "Trait std::iter::Iterator" and "1.0.0 · source" text blocks even more misaligned, I wonder if it's possible to have them on the same baseline and keep the button looking good.

@Alexendoo
Copy link
Member

The + is now very small, I find myself straining looking at it. Has the new design with the old size been tried?

image
image

@notriddle
Copy link
Contributor Author

@rfcbot concern bigger-plus-sign

@notriddle
Copy link
Contributor Author

notriddle commented Jul 3, 2023

Sounds good. I've pushed a version that's two pixels bigger.

old new
image image

@GuillaumeGomez
Copy link
Member

👍

@notriddle
Copy link
Contributor Author

I guess ❤️ means it fixed it.

@rfcbot resolve bigger-plus-sign

@bors
Copy link
Contributor

bors commented Jul 15, 2023

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

@notriddle
Copy link
Contributor Author

@Nemo157 I've tweaked the alignment of the out-of-band pane.

Before After
image image

@tgross35
Copy link
Contributor

tgross35 commented Oct 24, 2023

I think this fixes (or could reference) #60255

Re why-not-triangle: see also #107658

@bors
Copy link
Contributor

bors commented Oct 26, 2023

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

@camelid
Copy link
Member

camelid commented Dec 4, 2023

Small suggestion: Could you make the - aligned with the vertical center of the text? Currently it looks a little uneven:

image

@notriddle
Copy link
Contributor Author

@camelid Which one?

The biggest source of grief with trying to make the out-of-band section look good is that it's got three different font sizes. Should I align it with "source" or with "1.0.0"? Or should I tweak them so that they both have the same font size?

@GuillaumeGomez
Copy link
Member

I think this fixes (or could reference) #60255

Re why-not-triangle: see also #107658

We discussed about "why not triangle" yesterday at rustdoc meeting and the conclusion was as follows: we'd prefer to use triangles, but we can't find a nice way to make the "toggle/collapse all" button work with triangles. So for the time being, we'll continue to use the +/- buttons.

@GuillaumeGomez
Copy link
Member

@camelid Which one?

The biggest source of grief with trying to make the out-of-band section look good is that it's got three different font sizes. Should I align it with "source" or with "1.0.0"? Or should I tweak them so that they both have the same font size?

can you show screenshots of the different possibilities? Simpler to decide when we can actually see the result. :)

@notriddle
Copy link
Contributor Author

notriddle commented Dec 5, 2023

If I align it with the · bullet point, it looks like this:

image

If I align it with the 1.0.0 version number, it looks like this:

image

If I align it with the source link, it looks like this:

image

For comparison's sake, here's what it looks like now:

image

Personally, I think they all look terrible, and that this whole section should be redesigned to not use a bunch of different font sizes in a line like that.

@GuillaumeGomez
Copy link
Member

Agreed. Thanks for sharing the results. What do you think @camelid ?

This is a continuation of rust-lang#107658 and rust-lang#59851, making the toggles
consistent with other buttons, instead of looking like syntax.

The tactics to improve the look of these controls:

* When the toggle is expanded, the minus sign remains as dark as
  before, but the border is lighter. The plus sign has a border
  that's the same color as the label (black, on the default theme).
  This makes expansion buttons more prominent, easier to tell
  apart from collapse buttons.
* The plus sign is slightly taller than wide, also to make
  it easier to tell apart from the minus sign.
  * The use of crispEdges has to get a bit strategic to make this
    come out right. I've tested it on Firefox, Safari, and
    Chromium, but it's a bit hard to be sure it works right on
    all setups.
  * Does anybody know some trick to do crispEdges on only the
    X axis, while still allowing antialiasing on the Y?
* The "toggle all" button is modeled after the Help and Settings
  buttons, with a matching border, width, and +/− label.
@notriddle
Copy link
Contributor Author

I've uploaded a new version, rebased onto current master with a commit that changes other parts of the out-of-band bar.

Here's current nightly:

image

Here's a newer, nicer-looking-in-my-opinion version (:

image

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@GuillaumeGomez
Copy link
Member

I forgot to resolve concern in this comment.

@rfcbot resolve bigger-plus-sign

@GuillaumeGomez
Copy link
Member

Wrong concern...

@rfcbot resolve why-not-triangle

@GuillaumeGomez
Copy link
Member

Ah only original author can resolve the concern. Let's wait for @jsha then.

@jsha
Copy link
Contributor

jsha commented Feb 12, 2024

I'd still really like to figure out a way to make this work with triangles. It seems like we have common feeling on the team and elsewhere that triangles are the standard web way to make this type of interaction, and would be ideal. It sounds like the only thing blocking them is that the control in the top-right can't be a triangle (I agree), and if it's text, the layout moves around when the text changes. I think there are ways to solve those problems; for instance by making a text control that has a fixed size, and choosing words that are similar in size so that feels natural. Or even rethinking the top-right control entirely.

I feel bad holding up progress on something that has been more or less ready to go for a while now; I have been wanting to present a workable demo of the above ideas, and apologies for not dedicating the time so far to do so.

The reason I'm resistant to the incremental improvement now followed by a possible bigger improvement later is that the nature of docs.rs means we will have for a while some docs with the old [-]/[+] and some with the new, and later potentially with triangles. I think it's better to make our improvement all in one step so we can avoid unnecessary inconsistencies.

@notriddle
Copy link
Contributor Author

Obsoleted by #129545

@notriddle notriddle closed this Aug 26, 2024
@rfcbot rfcbot removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Aug 26, 2024
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.

Docs rs "[+]" ui element does not afford usage clearly