-
Notifications
You must be signed in to change notification settings - Fork 13k
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
Always display first line of impl blocks even when collapsed #132155
Conversation
Some changes occurred in HTML/CSS/JS. cc @GuillaumeGomez, @jsha |
This comment has been minimized.
This comment has been minimized.
30f487c
to
890fc0d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks cool 💚
src/librustdoc/html/markdown.rs
Outdated
@@ -1413,6 +1416,52 @@ impl MarkdownItemInfo<'_> { | |||
} | |||
} | |||
|
|||
pub(crate) fn markdown_split_summary_and_content( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should have a comment explaining what it does. Also, it should probably be a method on the Markdown
struct? It's basically a different version of to_string
.
/// Convert markdown to (summary, remaining) HTML.
///
/// - The summary is the first top-level Markdown element (usually a paragraph, but potentially any block).
/// - The remaining docs contain everything after the summary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good idea for the docs. I don't have an opinion on whether or not it should be a method so I'll just make a method since you seem to favor it. 👍
af1b4fd
to
391c707
Compare
Applied suggestions. |
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
This comment was marked as duplicate.
@notriddle proposal cancelled. |
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. |
left: 0; | ||
right: 0; | ||
pointer-events: none; | ||
background: linear-gradient( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't love the look of this. It looks a bit out of place compared to the generally clean style of rustdoc. In the case of tables and other things, it seems reasonable to just not show them rather than do a paywall-style blur of everything below the first row.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rfcbot concern gradient-looks-weird
I see what you mean there. To avoid this, I think you'd want to only have a summary if the first element is a paragraph or header, and then use white-space: nowrap; overflow: hidden; text-overflow: ellipsis;
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Exactly, that sounds good.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I liked the blur effect but text ellipsis is fine too. 😆
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can't figure a way to have an ellipsis which matches these conditions:
- Appears whatever the width of the first line.
- Appears only if there is hidden content.
With the blur I didn't have these issues, hence why I went for this solution. 😆
If you have an idea, I'd love to hear it!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few things I thought about: the problem with short sentences is that they don't overflow, and therefore don't generate the ellipsis. However if there is more content, users won't until expanded. So instead I tried to cheat a bit and added a ::after
element:
details.toggle:not([open]) > summary .docblock > :first-child {
max-width: calc(100% - 1em);
overflow: hidden;
width: fit-content;
white-space: nowrap;
position: relative;
text-overflow: clip;
padding-right: 1em;
}
details.toggle:not([open]) > summary .docblock > :first-child::after {
content: "…";
position: absolute;
right: 0;
top: 0;
background-color: var(--main-background-color);
}
However, it never gets displayed correctly on big text as it goes over it or is always stuck to the right side. I was hoping to kinda go around this issue by using padding
and was able to. It gives this result:
If you are ok with this result, I can go with it.
@rfcbot concern gradient-looks-weird |
5ff686a
to
2555bc1
Compare
Fixed merge conflicts. |
☔ The latest upstream changes (presumably #133345) made this pull request unmergeable. Please resolve the merge conflicts. |
2555bc1
to
55d9f59
Compare
This comment has been minimized.
This comment has been minimized.
The final comment period, with a disposition to merge, as per the review above, is now complete. As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed. This will be merged soon. |
1993e55
to
854ebe7
Compare
@bors r=rustdoc |
…llaumeGomez Rollup of 8 pull requests Successful merges: - rust-lang#132155 (Always display first line of impl blocks even when collapsed) - rust-lang#133256 (CI: use free runners for i686-gnu jobs) - rust-lang#133607 (implement checks for tail calls) - rust-lang#133821 (Replace black with ruff in `tidy`) - rust-lang#133827 (CI: rfl: move job forward to Linux v6.13-rc1) - rust-lang#133910 (Normalize target-cpus.rs stdout test for LLVM changes) - rust-lang#133921 (Adapt codegen tests for NUW inference) - rust-lang#133936 (Avoid fetching the anon const hir node that is already available) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#132155 - GuillaumeGomez:impl-block-doc, r=rustdoc Always display first line of impl blocks even when collapsed Fixes rust-lang#130612. It the line is too long, only the beginning will be visible: ![Screenshot from 2024-10-25 17-21-41](/~https://github.com/user-attachments/assets/dd2d912c-ad55-4410-8195-1d66a0a99ad4) Otherwise, it looks like this: ![image](/~https://github.com/user-attachments/assets/1f40b9e0-2143-4b9d-a4b0-338a0cd740df) Can be tested [here](https://rustdoc.crud.net/imperio/impl-block-doc/foo/struct.ImplDoc.html). r? `@notriddle`
Fixes #130612.
It the line is too long, only the beginning will be visible:
Otherwise, it looks like this:
Can be tested here.
r? @notriddle