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: change the +/- icons #107658

Closed
wants to merge 3 commits into from
Closed

Conversation

rol1510
Copy link
Contributor

@rol1510 rol1510 commented Feb 4, 2023

The [+] and [-] icons to expand/collapse parts of the documentation are really difficult to tell apart. This tries to replace them with something simpler.

Old
image

New
image
image
image

Demo: https://host-7e0a2.web.app/sample-2/std/iter/trait.Iterator.html

Also discussed here: #59851

@rustbot
Copy link
Collaborator

rustbot commented Feb 4, 2023

r? @notriddle

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

rustbot commented Feb 4, 2023

Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez, @Folyd, @jsha

@notriddle
Copy link
Contributor

Personally, I'm okay with this, but I'd like the toggle-all-docs button in the top right corner to be redesigned as well. It should probably also go through FCP before merging it.

@GuillaumeGomez
Copy link
Member

I also like the new icon and I agree that we need an FCP for it.

Can you provide screenshots for the other themes as well please?

@tgross35
Copy link
Contributor

tgross35 commented Feb 5, 2023

(nit) It looks like for the the methods and description dropdown, the arrow appears to be slightly below the line of text. Compared to function & type items where it's centered

image

image

(also nit) A slight size reduction to about 90% of line height might look good, just to reduce its prominence compared to the text it's associated with

@rol1510
Copy link
Contributor Author

rol1510 commented Feb 6, 2023

The toggle-all-docs button now has a new icon. I also made the other icons a little smaller and aligned them properly.

@GuillaumeGomez
Copy link
Member

I'm not a big fan of the new "toggle all" button. At least the previous was somehow explicit about what it was doing. The new one is more beautiful but more difficult to decipher (imo).

@rust-log-analyzer
Copy link
Collaborator

The job x86_64-gnu-tools failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
Prepare all required actions
Getting action download info
Download action repository 'actions/checkout@v3' (SHA:ac593985615ec2ede58e132d2e21d2b1cbd6127c)
Download action repository 'rust-lang/simpleinfra@master' (SHA:aa723573e04016ede7da6c5d7b029e72cb8a05a3)
Complete job name: PR (x86_64-gnu-tools, false, 1, ubuntu-20.04-xl)
git config --global core.autocrlf false
shell: /usr/bin/bash --noprofile --norc -e -o pipefail {0}
env:
  CI_JOB_NAME: x86_64-gnu-tools
---
  IMAGE: x86_64-gnu-tools
##[endgroup]
From https://github.com/rust-lang/rust
 * branch              master     -> FETCH_HEAD
Searching for toolstate changes between 7c3f0d6f30eeefa58170012df10008736ac89755 and d1d85508897c4f3eac594a495858072c94a0ae22
Rustdoc was updated
##[group]Run src/ci/scripts/verify-channel.sh
src/ci/scripts/verify-channel.sh
shell: /usr/bin/bash --noprofile --norc -e -o pipefail {0}
env:
---
.......... (90/97)
...    (97/97)


/checkout/tests/rustdoc-gui/scrape-examples-toggle.goml scrape-examples-toggle... FAILED
[ERROR] (around line 46): JS errors occurred: Error: TypeError: Cannot set properties of undefined (setting 'innerText')
    at collapseAllDocs (file:///checkout/obj/build/x86_64-unknown-linux-gnu/test/rustdoc-gui/doc/static.files/main-364b95f387bc3166.js:1:12034)
    at HTMLButtonElement.toggleAllDocs (file:///checkout/obj/build/x86_64-unknown-linux-gnu/test/rustdoc-gui/doc/static.files/main-364b95f387bc3166.js:1:12208)
/checkout/tests/rustdoc-gui/shortcuts.goml shortcuts... FAILED
[ERROR] (line 16) Error: Evaluation failed: The following errors happened: [`` isn't equal to `[−]`]: for command `assert-text: ("#toggle-all-docs", "[\u2212]")`
[ERROR] (around line 17): JS errors occurred: Error: TypeError: Cannot set properties of undefined (setting 'innerText')
    at collapseAllDocs (file:///checkout/obj/build/x86_64-unknown-linux-gnu/test/rustdoc-gui/doc/static.files/main-364b95f387bc3166.js:1:12034)
    at HTMLDocument.handleShortcut (file:///checkout/obj/build/x86_64-unknown-linux-gnu/test/rustdoc-gui/doc/static.files/main-364b95f387bc3166.js:1:7085)
/checkout/tests/rustdoc-gui/sidebar.goml sidebar... FAILED
[ERROR] (around line 146): JS errors occurred: Error: TypeError: Cannot set properties of undefined (setting 'innerText')
    at collapseAllDocs (file:///checkout/obj/build/x86_64-unknown-linux-gnu/test/rustdoc-gui/doc/static.files/main-364b95f387bc3166.js:1:12034)
    at HTMLButtonElement.toggleAllDocs (file:///checkout/obj/build/x86_64-unknown-linux-gnu/test/rustdoc-gui/doc/static.files/main-364b95f387bc3166.js:1:12208)
/checkout/tests/rustdoc-gui/toggle-docs.goml toggle-docs... FAILED
[ERROR] (line 4) Error: Evaluation failed: The following errors happened: [`` isn't equal to `[−]`]: for command `assert-text: ("#toggle-all-docs", "[−]")`
[ERROR] (around line 5): JS errors occurred: Error: TypeError: Cannot set properties of undefined (setting 'innerText')
    at collapseAllDocs (file:///checkout/obj/build/x86_64-unknown-linux-gnu/test/rustdoc-gui/doc/static.files/main-364b95f387bc3166.js:1:12034)
    at HTMLButtonElement.toggleAllDocs (file:///checkout/obj/build/x86_64-unknown-linux-gnu/test/rustdoc-gui/doc/static.files/main-364b95f387bc3166.js:1:12208)

@jsha
Copy link
Contributor

jsha commented Feb 6, 2023

Re: the toggle button, I think the most explicit thing would be to replace the icon with a word - summary in the default state, or expand after everything has been collapsed.

@rol1510
Copy link
Contributor Author

rol1510 commented Feb 8, 2023

I like @jsha's Idea. Looks great and is very explicit.

52
55
54
56

@GuillaumeGomez
Copy link
Member

I like it too. Should we use the same color as for the source link right beside it?

@rol1510
Copy link
Contributor Author

rol1510 commented Feb 8, 2023

I feel like the gray looks a little better. Also, the color of source is the link color, which could be confusing.

63

@GuillaumeGomez
Copy link
Member

But grey is associated with "disabled item". So I don't think it's a good idea to keep as is.

@rol1510
Copy link
Contributor Author

rol1510 commented Feb 8, 2023

Good Point. We could set it to the main color

68
69
70

@rol1510
Copy link
Contributor Author

rol1510 commented Feb 8, 2023

A border around it might also work

72

@notriddle
Copy link
Contributor

notriddle commented Feb 8, 2023

  1. The differing font sizes look weird to me. Either use a border, or use the same font size as "source".
  2. Since the words "expand" and "collapse" don't necessarily have the same width, clicking the button will probably cause the stuff next to it to move, which also looks weird.

@rol1510
Copy link
Contributor Author

rol1510 commented Feb 9, 2023

maybe something like that?

Frame 8

@GuillaumeGomez
Copy link
Member

The arrows are making me think it can be downloaded. As for the +/-, isn't it basically the same as currently?

@bors
Copy link
Contributor

bors commented Mar 7, 2023

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

@rol1510 rol1510 closed this Mar 7, 2023
@tgross35
Copy link
Contributor

tgross35 commented Mar 7, 2023

If this has been decided against, it probably makes sense to post an update to #59851. But maybe Just making the + and - bold would be enough to address the original listed "difficult to tell apart" concern?

notriddle added a commit to notriddle/rust that referenced this pull request Aug 10, 2023
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 added a commit to notriddle/rust that referenced this pull request Sep 20, 2023
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 added a commit to notriddle/rust that referenced this pull request Dec 6, 2023
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.
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.

8 participants