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

Wrap too long type name #127418

Merged
merged 2 commits into from
Jul 19, 2024
Merged

Conversation

GuillaumeGomez
Copy link
Member

Fixes #120595.

Takeover of #126209.

cc @BradMarr
r? @notriddle

@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 Jul 6, 2024
@rustbot
Copy link
Collaborator

rustbot commented Jul 6, 2024

Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez, @jsha

@rust-log-analyzer

This comment has been minimized.

@GuillaumeGomez
Copy link
Member Author

Weird, different size locally vs in CI. Oh well...

Copy link
Contributor

@notriddle notriddle left a comment

Choose a reason for hiding this comment

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

Okay, it's really, really important to include a screenshot on a change that's this drastic:

Don't like it.

image

Also:

  • If the text is going to be aligned to the right, it should be consistently aligned to the right. Notice, in the above screenshot, array is ragged-right, but slice is ragged-left.

  • Even if it is consistent, that's merely better. It's not actually good. Large amounts of English, like these descriptions, should always be written left-to-right.

padding: 0;
margin: 0;
}
.item-table > li {
display: table-row;
display: flex;
justify-content: space-between;
}
.item-table > li > div {
display: table-cell;
Copy link
Contributor

Choose a reason for hiding this comment

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

This can't actually do anything any more.

@rustbot rustbot 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-review Status: Awaiting review from the assignee but also interested parties. labels Jul 6, 2024
@BradMarr
Copy link

BradMarr commented Jul 6, 2024

Yeah that's what I figured, I couldn't figure out how to do it the way it was and having a max width.

@the8472
Copy link
Member

the8472 commented Jul 7, 2024

#120595 (comment) cites table layout intrinsic width issues.

I'm not sure if that's correct. The issue seems to be that the table columns have different overflow-wrap rules which I think leads one column to hog all the space at the expense of the other. If both are set to anywhere then it balances the available space and it looks fine(-ish). It could use some polish to indicate that wrapping happened, a dash or an indent on the 2nd line.

image

@GuillaumeGomez
Copy link
Member Author

Finally took the time to update. I reverted back to the table display but with some small differences: there is now a padding at the bottom to make it easier to spot which item's short doc we're reading and also set the width of the items name fixed (to 50% of the parent) so that whatever their size, the display remains the same. Screenshots:

Screenshot from 2024-07-17 14-27-33
Screenshot from 2024-07-17 14-27-52
Screenshot from 2024-07-17 14-29-52

@GuillaumeGomez
Copy link
Member Author

I'll add GUI tests once we agree with the UI changes.

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jul 17, 2024
@rust-log-analyzer

This comment has been minimized.

@notriddle notriddle added T-rustdoc-frontend Relevant to the rustdoc-frontend team, which will review and decide on the web UI/UX output. and removed T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Jul 17, 2024
@notriddle
Copy link
Contributor

Does @the8472's solution work? It might look better than the fixed-width setup with huge gaps.

@GuillaumeGomez
Copy link
Member Author

If one item is super long and the others are "normal", then the rendering is the same. One thing I dislike with the current display is that the position of the short documentation changes depending on the width of the biggest item name. That's why I decided to go for fixed width (with still handling too long name).

@notriddle
Copy link
Contributor

Okay, that makes sense.

Next question: instead of making the name 50% of the size, make it 33%? Item names are usually a lot shorter than descriptions, and the huge blank space between them makes it hard to match them up (especially since there’s no zebra striping on this table).

@GuillaumeGomez
Copy link
Member Author

That's a good point. Gonna add UI tests to this new size then.

@GuillaumeGomez
Copy link
Member Author

Reduced the width of items name to 33% and added a GUI test.

@notriddle
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Jul 18, 2024

📌 Commit c820a23 has been approved by notriddle

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 Jul 18, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 19, 2024
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#127418 (Wrap too long type name)
 - rust-lang#127594 (Fuchsia status code match arm)
 - rust-lang#127835 (Fix ICE in suggestion caused by `⩵` being recovered as `==`)
 - rust-lang#127858 (match lowering: Rename `MatchPair` to `MatchPairTree`)
 - rust-lang#127871 (Mention that type parameters are used recursively on bivariance error)
 - rust-lang#127913 (remove `debug-logging` default from tools profile)
 - rust-lang#127925 (Remove tag field from `Relation`s)
 - rust-lang#127929 (Use more accurate span for `addr_of!` suggestion)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 19, 2024
…iaskrgr

Rollup of 8 pull requests

Successful merges:

 - rust-lang#127418 (Wrap too long type name)
 - rust-lang#127594 (Fuchsia status code match arm)
 - rust-lang#127835 (Fix ICE in suggestion caused by `⩵` being recovered as `==`)
 - rust-lang#127858 (match lowering: Rename `MatchPair` to `MatchPairTree`)
 - rust-lang#127871 (Mention that type parameters are used recursively on bivariance error)
 - rust-lang#127913 (remove `debug-logging` default from tools profile)
 - rust-lang#127925 (Remove tag field from `Relation`s)
 - rust-lang#127929 (Use more accurate span for `addr_of!` suggestion)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 19, 2024
Rollup of 8 pull requests

Successful merges:

 - rust-lang#127418 (Wrap too long type name)
 - rust-lang#127594 (Fuchsia status code match arm)
 - rust-lang#127835 (Fix ICE in suggestion caused by `⩵` being recovered as `==`)
 - rust-lang#127858 (match lowering: Rename `MatchPair` to `MatchPairTree`)
 - rust-lang#127871 (Mention that type parameters are used recursively on bivariance error)
 - rust-lang#127913 (remove `debug-logging` default from tools profile)
 - rust-lang#127925 (Remove tag field from `Relation`s)
 - rust-lang#127929 (Use more accurate span for `addr_of!` suggestion)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 7c1bf86 into rust-lang:master Jul 19, 2024
6 checks passed
@rustbot rustbot added this to the 1.81.0 milestone Jul 19, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jul 19, 2024
Rollup merge of rust-lang#127418 - GuillaumeGomez:wrap-too-long-type-name, r=notriddle

Wrap too long type name

Fixes rust-lang#120595.

Takeover of rust-lang#126209.

cc `@BradMarr`
r? `@notriddle`
@GuillaumeGomez GuillaumeGomez deleted the wrap-too-long-type-name branch July 19, 2024 08:52
width: 33%;
}
.item-table > li > div {
padding-bottom: 5px;
Copy link
Contributor

@cynecx cynecx Jul 22, 2024

Choose a reason for hiding this comment

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

@GuillaumeGomez Maybe it's just me but this makes the spacing in the y-axis look so "wasteful" and a bit unnatural. Is this really necessary? Or why can't we keep the old behavior.

Copy link
Contributor

Choose a reason for hiding this comment

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

tgross35 added a commit to tgross35/rust that referenced this pull request Jul 22, 2024
…llaumeGomez

rustdoc: revert spacing change in item-table

It really wasn't necessary for the bug fix, and could reasonably be considered a functional regression.

In response to rust-lang#127418 (comment)
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jul 22, 2024
Rollup merge of rust-lang#128051 - notriddle:notriddle/spacing, r=GuillaumeGomez

rustdoc: revert spacing change in item-table

It really wasn't necessary for the bug fix, and could reasonably be considered a functional regression.

In response to rust-lang#127418 (comment)
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-frontend Relevant to the rustdoc-frontend team, which will review and decide on the web UI/UX output.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

rustdoc: Long type names result in weird-looking documentation
8 participants