-
Notifications
You must be signed in to change notification settings - Fork 12.8k
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
base: master
Are you sure you want to change the base?
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 |
1cc40fa
to
4fc3871
Compare
I pushed the change with the text ellipsis. I find this solution less good but if majority prefers it, let's go with it. :) |
Half+1 members approved the PR. Should we move forward with it? |
☔ The latest upstream changes (presumably #133006) made this pull request unmergeable. Please resolve the merge conflicts. |
4fc3871
to
6690b22
Compare
Fixed merge conflicts. |
@rfcbot resolve gradient-looks-weird |
🔔 This is now entering its final comment period, as per the review above. 🔔 |
☔ The latest upstream changes (presumably #133047) made this pull request unmergeable. Please resolve the merge conflicts. |
6690b22
to
5943329
Compare
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