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: Align search results horizontally for easy scanning #112937

Merged
merged 5 commits into from
Jun 25, 2023

Conversation

camelid
Copy link
Member

@camelid camelid commented Jun 22, 2023

The recent PR #110688 added info about an item's kind before its name in
search results. However, because the kind and name are inline with no
alignment, it's now hard to visually scan downward through the search
results, looking at item names. This PR fixes that by horizontally
aligning search results such that there are now two columns of
information.

r? @GuillaumeGomez

The recent PR rust-lang#110688 added info about an item's kind before its name in
search results. However, because the kind and name are inline with no
alignment, it's now hard to visually scan downward through the search
results, looking at item names. This PR fixes that by horizontally
aligning search results such that there are now two columns of
information.
@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 22, 2023
@compiler-errors
Copy link
Member

Do you have a rendering of what the new results look like?

Also, quite in favor of this change 👍

@camelid
Copy link
Member Author

camelid commented Jun 22, 2023

EDIT: This version is out-of-date. Please see #112937 (comment) for the final version.

Note: I'm having an issue getting Rust to build on my computer right now, so the "after" screenshot was generated by applying the changes in Firefox dev tools.

Before

image

After

image

color: var(--search-results-grey-color);
font-size: 0.875rem;
width: 6rem;
Copy link
Member Author

Choose a reason for hiding this comment

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

I picked this number arbitrarily. Two issues:

  1. It would be nice if it weren't based on eyeballing the layout.

  2. This isn't actually wide enough for "associated constant". If I increase this to 9em, it fits, but then all the other rows have excessive spacing. I see two solutions: one is to shorten the typename to just "constant", the other is to make the spacing somehow adaptive to what the longest typename is. But I'm not sure how to implement that.

Copy link
Member

Choose a reason for hiding this comment

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

What happens when it overflows? Does it wrap and just become a taller row? I think that honestly wouldn't be terrible if that's what happens.

Copy link
Member Author

Choose a reason for hiding this comment

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

It's not terrible, but still looks bad IMO:

image

Copy link
Member

Choose a reason for hiding this comment

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

:/ true -- i wonder if vertically center-aligning those rows would help.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we really need to just keep rows a constant height. Variable height rows almost always look a bit off.

Copy link
Member

Choose a reason for hiding this comment

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

Something that could at least make them look better is to make sure both the path/name and short description are aligned the same, to the top or the middle as Michael suggested.

Copy link
Member Author

Choose a reason for hiding this comment

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

I was able to avoid the wrapping by adding a tiny bit extra space.

@camelid camelid changed the title Align search results horizontally for easy scanning rustdoc: Align search results horizontally for easy scanning Jun 22, 2023
@GuillaumeGomez
Copy link
Member

The longest type name is associated constant. So calc(0.875rem * 20) should do the trick. But it's quite wide, hence why we finally decided to go without it. UI is tricky. ^^'

@camelid
Copy link
Member Author

camelid commented Jun 23, 2023

@GuillaumeGomez could we shorten "associated constant" to just "constant"? I'm not sure if it really matters to users whether a constant is associated or free, and even aside from this change, it's quite a long word that distracts from the actual name of the item.

@compiler-errors
Copy link
Member

could we shorten "associated constant" to just "constant"?

I'd honestly rather shorten it to assoc const or something. At least on the compiler side, we try to be consistent with calling things "associated".

@camelid
Copy link
Member Author

camelid commented Jun 23, 2023

Abbreviation would be fine with me 👍

@GuillaumeGomez
Copy link
Member

GuillaumeGomez commented Jun 23, 2023

But associated const is already super long (especially if no type kind this long is displayed), and also it would become the second longest after existential type. But we can already try to see how it looks with assoc const already?

EDIT: It's actually pretty useless to do since existential type is much longer and I don't think we can shorten it.

@camelid
Copy link
Member Author

camelid commented Jun 23, 2023

I abbreviated associated type too (not visible here):

image

@GuillaumeGomez
Copy link
Member

It looks nice like that. But remains the problem of existential type.

@camelid
Copy link
Member Author

camelid commented Jun 23, 2023

Existential types have not been implemented yet I thought? Or I thought they're at least an experimental feature.

@camelid
Copy link
Member Author

camelid commented Jun 23, 2023

"Existential type" is still shorter than "associated constant". We could shorten "existential type" to just "existential"?

What is an existential type in this context actually? Is it type Foo = impl Trait?

@compiler-errors
Copy link
Member

@camelid: type alias impl trait ("existential type" in rustdoc i guess) is implemented, and we probably should account for them too

@camelid
Copy link
Member Author

camelid commented Jun 23, 2023

We'd actually only need 7 rem (rather the current 6) to support "existential type" as it is.

@GuillaumeGomez
Copy link
Member

Why 7rem? "existential type" is 17 characters. So 17 * 0.875 = 14.875rem, not 7. Did I miss something?

@camelid
Copy link
Member Author

camelid commented Jun 23, 2023

In non-monospace fonts, the letters are variable width, so you can't just multiply the font size by the number of characters. In addition, I believe the "em" in "rem" is referring to a value that is (approximately, sigh...) the width of an uppercase "M", which should be wider than the letters we're using.

EDIT: Here's a demo of two strings with the same length but different characters (16 M's, which I believe is the correct length of "existential type"):

image

Note: In case anyone's confused by "u8" being an existential type: I'm using browser dev tools to create this image ;)

@camelid
Copy link
Member Author

camelid commented Jun 23, 2023

New screenshots. Let me know if there are any other types of items that you'd like to see rendered.

image

image

@GuillaumeGomez
Copy link
Member

Can you just check what it gives with "existential type" please? (Just update the text, no need for a real one 😸 )

Also please show screenshots of a mobile view (just reduce the width).

@camelid
Copy link
Member Author

camelid commented Jun 23, 2023

By the way, I added a demo to my "rem" explanation above in case you're interested :)

"existential type":

image

mobile view:

image

@GuillaumeGomez
Copy link
Member

Let's maybe reduce a bit the margin after the type name? But otherwise looks good to me. 👍

@camelid camelid marked this pull request as ready for review June 23, 2023 17:30
@rustbot
Copy link
Collaborator

rustbot commented Jun 23, 2023

Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez, @Folyd, @jsha

@camelid
Copy link
Member Author

camelid commented Jun 23, 2023

Some options:

6.25 rem

IMO the "existential type" is too close to the name.

image

6.5 rem

image

6.75 rem

image

7 rem

image

@compiler-errors
Copy link
Member

👍

@camelid
Copy link
Member Author

camelid commented Jun 24, 2023

@bors r=GuillaumeGomez

@bors
Copy link
Contributor

bors commented Jun 24, 2023

📌 Commit 5f433f3 has been approved by GuillaumeGomez

It is now in the queue for this repository.

@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 Jun 24, 2023
@camelid
Copy link
Member Author

camelid commented Jun 24, 2023

@bors rollup

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Jun 24, 2023
…meGomez

rustdoc: Align search results horizontally for easy scanning

The recent PR rust-lang#110688 added info about an item's kind before its name in
search results. However, because the kind and name are inline with no
alignment, it's now hard to visually scan downward through the search
results, looking at item names. This PR fixes that by horizontally
aligning search results such that there are now two columns of
information.

r? `@GuillaumeGomez`
@GuillaumeGomez
Copy link
Member

Failed in #113004 (comment). You forgot to update a GUI test. ;)

@bors r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jun 24, 2023
@rustbot
Copy link
Collaborator

rustbot commented Jun 24, 2023

Some changes occurred in GUI tests.

cc @GuillaumeGomez

@GuillaumeGomez
Copy link
Member

Thanks!

@bors r=notriddle,GuillaumeGomez rollup

@bors
Copy link
Contributor

bors commented Jun 24, 2023

📌 Commit 9b97ae1 has been approved by notriddle,GuillaumeGomez

It is now in the queue for this repository.

@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 Jun 24, 2023
@notriddle
Copy link
Contributor

@bors r=notriddle,GuillaumeGomez rollup

@bors
Copy link
Contributor

bors commented Jun 24, 2023

💡 This pull request was already approved, no need to approve it again.

@bors
Copy link
Contributor

bors commented Jun 24, 2023

📌 Commit 9b97ae1 has been approved by notriddle,GuillaumeGomez

It is now in the queue for this repository.

bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 25, 2023
…iaskrgr

Rollup of 5 pull requests

Successful merges:

 - rust-lang#112937 (rustdoc: Align search results horizontally for easy scanning)
 - rust-lang#112950 (DirEntry::file_name: improve explanation)
 - rust-lang#112956 (Expose `compiler-builtins-weak-intrinsics` feature for `-Zbuild-std`)
 - rust-lang#113008 (Move some docs from the README to the dev-guide)
 - rust-lang#113009 (Remove unnecessary `path` attribute)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 8816f9e into rust-lang:master Jun 25, 2023
@rustbot rustbot added this to the 1.72.0 milestone Jun 25, 2023
@camelid camelid deleted the align-typenames branch June 25, 2023 16:04
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.

7 participants