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

Update rustdoc.css #126209

Closed
wants to merge 8 commits into from
Closed

Update rustdoc.css #126209

wants to merge 8 commits into from

Conversation

BradMarr
Copy link

@BradMarr BradMarr commented Jun 9, 2024

fixes #120595

removing this selector fixes weird wrapping in certain pages like https://docs.rs/async-stripe/0.31.0/stripe/index.html#structs

@rustbot
Copy link
Collaborator

rustbot commented Jun 9, 2024

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @GuillaumeGomez (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

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

rustbot commented Jun 9, 2024

Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez, @jsha

@GuillaumeGomez
Copy link
Member

This indeed seems to fix the issue, thanks!

Please add a test in tests/rustdoc-gui to ensure this bug won't appear again later on. If you need help to write the test, don't hesitate to ask!

@BradMarr
Copy link
Author

BradMarr commented Jun 9, 2024

I'm not familiar with the test system and don't know I could prevent this issue from occurring in the future. The issue was simply that the property was spread to an element it shouldn't have been so I don't know if a test could detect a problem in the future without knowing the intention of the programmer. I'm very new to this though and could be completely incorrect.

@rust-log-analyzer

This comment has been minimized.

@BradMarr
Copy link
Author

BradMarr commented Jun 9, 2024

I'm lost as to how an offset error would occur when my only change was to the overflow-wrap.

@rustbot rustbot added has-merge-commits PR has merge commits, merge with caution. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 10, 2024
@rustbot
Copy link
Collaborator

rustbot commented Jun 10, 2024

There are merge commits (commits with multiple parents) in your changes. We have a no merge policy so these commits will need to be removed for this pull request to be merged.

You can start a rebase with the following commands:

$ # rebase
$ git pull --rebase https://github.com/rust-lang/rust.git master
$ git push --force-with-lease

The following commits are merge commits:

@notriddle
Copy link
Contributor

I don't think that this is a good fix. The problem is that it improves the async-stripe API, but it makes other APIs look worse. For example, this is what std::marker looks like in Firefox. First without the change, then with it.

Before

image

After

image

@rust-log-analyzer

This comment has been minimized.

@BradMarr
Copy link
Author

BradMarr commented Jun 10, 2024

maybe we could do a minimum width along with the default method so that it only makes the lines overlap if it would otherwise create the vertical line bug @notriddle

@notriddle
Copy link
Contributor

Maybe we can automatically insert <wbr> tags between CamelCase and snake_case words?

@BradMarr
Copy link
Author

BradMarr commented Jun 10, 2024

That would require parsing through all of the words, I think maybe giving each of the .item-table>li>.item-name a max width of half of the width of item-table would allow for overflow of the item-name but only if it would otherwise cause a bug

@BradMarr
Copy link
Author

How does that commit look?

@rust-log-analyzer

This comment has been minimized.

@@ -898,8 +898,8 @@ because of the `[-]` element which would overlap with it. */
So, for table layouts, override the default with break-word, which does
_not_ affect min-content intrinsic sizes.
*/
table,
.item-table {
.item-table,
Copy link
Member

Choose a reason for hiding this comment

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

This change can be removed.

@rust-log-analyzer

This comment has been minimized.

@BradMarr
Copy link
Author

I'm confused on how to fix the check issues.

@GuillaumeGomez
Copy link
Member

It says that in tests/rustdoc-gui/type-declation-overflow.goml at line 19, if expected a different value for the offsetWidth element property. Since you modified it, seems logical that it fails.

@BradMarr
Copy link
Author

It says that in tests/rustdoc-gui/type-declation-overflow.goml at line 19, if expected a different value for the offsetWidth element property. Since you modified it, seems logical that it fails.

How should I go about fixing that?

@GuillaumeGomez
Copy link
Member

Update the mentioned file.

@BradMarr
Copy link
Author

Update the mentioned file.

The mentioned file being the test or the css page?

@GuillaumeGomez
Copy link
Member

The test.

@rustbot
Copy link
Collaborator

rustbot commented Jun 10, 2024

Some changes occurred in GUI tests.

cc @GuillaumeGomez

@BradMarr
Copy link
Author

BradMarr commented Jun 13, 2024

What if we did something like this, where instead of a table it is a flex? It does look a little different but I don't mind the appearance.

@rust-log-analyzer

This comment has been minimized.

@GuillaumeGomez
Copy link
Member

The failure is "normal", not related to your changes. I restarted failing checks. If it still fails with the same error, just rebase.

clubby789 and others added 7 commits June 28, 2024 21:19
The type name ID map has underscores in its names, so the query
element should have them, too.
- Autolabel PRs modifying `tests/run-make/` and
  `src/tools/run-make-support/` with `X-run-make` label.
- Add reminder to update the tracking issue
  <#121876>
  if applicable when `tests/run-make/` is modified by a PR.
Update rustdoc.css

Update rustdoc.css

Update rustdoc.css

Update rustdoc.css

Update rustdoc.css

Update rustdoc.css

Update type-declation-overflow.goml

fix test

make into flex
@rustbot rustbot added A-meta Area: Issues & PRs about the rust-lang/rust repository itself A-testsuite Area: The testsuite used to check the correctness of rustc O-windows Operating system: Windows T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. labels Jun 29, 2024
@rustbot
Copy link
Collaborator

rustbot commented Jun 29, 2024

Some changes occurred in src/tools/clippy

cc @rust-lang/clippy

triagebot.toml has been modified, there may have been changes to the review queue.

cc @davidtwco, @wesleywiser

The list of allowed third-party dependencies may have been modified! You must ensure that any new dependencies have compatible licenses before merging.

cc @davidtwco, @wesleywiser

These commits modify the Cargo.lock file. Unintentional changes to Cargo.lock can be introduced when switching branches and rebasing PRs.

If this was unintentional then you should revert the changes before this PR is merged.
Otherwise, you can ignore this comment.

@rustbot rustbot removed has-merge-commits PR has merge commits, merge with caution. S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 29, 2024
@GuillaumeGomez
Copy link
Member

You messed up the git history. ^^'

@BradMarr
Copy link
Author

BradMarr commented Jul 5, 2024

I think I'm a little bit out of my depth here, somebody else should probably take the reins. Thanks for your patience you guys.

If there's anything I can do to make it easier for you guys, I can do that for you.

@GuillaumeGomez
Copy link
Member

I'll take a look tomorrow.

@GuillaumeGomez
Copy link
Member

Closing in favor of #127418.

@notriddle notriddle closed this Jul 6, 2024
@BradMarr BradMarr deleted the fix-weird-text-wrapping-for-long-type-names branch July 6, 2024 16:10
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jul 18, 2024
…name, r=notriddle

Wrap too long type name

Fixes rust-lang#120595.

Takeover of rust-lang#126209.

cc `@BradMarr`
r? `@notriddle`
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`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jul 29, 2024
…-table, r=GuillaumeGomez

rustdoc: word wrap CamelCase in the item list table and sidebar

This is an alternative to rust-lang#126209. That is, it fixes the issue that affects the very long type names in https://docs.rs/async-stripe/0.31.0/stripe/index.html#structs.

This is, necessarily, a pile of nasty heuristics. We need to balance a few issues:

- Sometimes, there's no real word break. For example, `BTreeMap` should be `BTree<wbr>Map`, not `B<wbr>Tree<wbr>Map`.

- Sometimes, there's a legit word break, but the name is tiny and the HTML overhead isn't worth it. For example, if we're typesetting `TyCtx`, writing `Ty<wbr>Ctx` would have an HTML overhead of 50%. Line breaking inside it makes no sense.

# Screenshots

| Before | After |
| ------ | ----- |
| ![image](https://github.com/rust-lang/rust/assets/1593513/d51201fd-46c0-4f48-aee6-a477eadba288) | ![image](https://github.com/rust-lang/rust/assets/1593513/d8e77582-adcf-4966-bbfd-19dfdad7336a)
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jul 29, 2024
…-table, r=GuillaumeGomez

rustdoc: word wrap CamelCase in the item list table and sidebar

This is an alternative to rust-lang#126209. That is, it fixes the issue that affects the very long type names in https://docs.rs/async-stripe/0.31.0/stripe/index.html#structs.

This is, necessarily, a pile of nasty heuristics. We need to balance a few issues:

- Sometimes, there's no real word break. For example, `BTreeMap` should be `BTree<wbr>Map`, not `B<wbr>Tree<wbr>Map`.

- Sometimes, there's a legit word break, but the name is tiny and the HTML overhead isn't worth it. For example, if we're typesetting `TyCtx`, writing `Ty<wbr>Ctx` would have an HTML overhead of 50%. Line breaking inside it makes no sense.

# Screenshots

| Before | After |
| ------ | ----- |
| ![image](https://github.com/rust-lang/rust/assets/1593513/d51201fd-46c0-4f48-aee6-a477eadba288) | ![image](https://github.com/rust-lang/rust/assets/1593513/d8e77582-adcf-4966-bbfd-19dfdad7336a)
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jul 30, 2024
Rollup merge of rust-lang#126247 - notriddle:notriddle/word-wrap-item-table, r=GuillaumeGomez

rustdoc: word wrap CamelCase in the item list table and sidebar

This is an alternative to rust-lang#126209. That is, it fixes the issue that affects the very long type names in https://docs.rs/async-stripe/0.31.0/stripe/index.html#structs.

This is, necessarily, a pile of nasty heuristics. We need to balance a few issues:

- Sometimes, there's no real word break. For example, `BTreeMap` should be `BTree<wbr>Map`, not `B<wbr>Tree<wbr>Map`.

- Sometimes, there's a legit word break, but the name is tiny and the HTML overhead isn't worth it. For example, if we're typesetting `TyCtx`, writing `Ty<wbr>Ctx` would have an HTML overhead of 50%. Line breaking inside it makes no sense.

# Screenshots

| Before | After |
| ------ | ----- |
| ![image](https://github.com/rust-lang/rust/assets/1593513/d51201fd-46c0-4f48-aee6-a477eadba288) | ![image](https://github.com/rust-lang/rust/assets/1593513/d8e77582-adcf-4966-bbfd-19dfdad7336a)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-meta Area: Issues & PRs about the rust-lang/rust repository itself A-testsuite Area: The testsuite used to check the correctness of rustc O-windows Operating system: Windows S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. 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.

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