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

Improve Rustdoc UI for scraped examples with multiline arguments, fix overflow in line numbers #93217

Merged
merged 10 commits into from
Apr 13, 2022

Conversation

willcrichton
Copy link
Contributor

@willcrichton willcrichton commented Jan 22, 2022

This PR improves a few aspects of the scrape examples feature in Rustdoc.

  • Only function names and not the full call expression are highlighted.
  • For call-sites with multiline arguments, the minimized code viewer will scroll to the top of the call-site rather than the middle if the argument is larger than the viewer size, ensuring that the function name is visible.
  • This fixes an issue where the line numbers column had a visible x-scroll bar.

r? @GuillaumeGomez

@rust-highfive
Copy link
Collaborator

Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez,@Folyd

@rustbot rustbot added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Jan 22, 2022
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 22, 2022
@camelid camelid self-assigned this Jan 25, 2022
@camelid camelid 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 Jan 25, 2022
@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@rust-log-analyzer

This comment has been minimized.

@camelid
Copy link
Member

camelid commented Jan 31, 2022

Please move the --test commit to a new PR. It's a separate issue from improving the UI.

@camelid
Copy link
Member

camelid commented Jan 31, 2022

I think that change would also require an FCP and more discussion.

@willcrichton
Copy link
Contributor Author

Ok, I pulled that change into #93497.

@bors
Copy link
Contributor

bors commented Jan 31, 2022

☔ The latest upstream changes (presumably #93498) made this pull request unmergeable. Please resolve the merge conflicts.

@@ -2577,7 +2578,9 @@ fn render_call_locations(w: &mut Buffer, cx: &Context<'_>, item: &clean::Item) {
<span></span>\
<h5 id=\"{id}\" class=\"section-header\">\
<a href=\"#{id}\">Examples found in repository</a>\
<a class=\"scrape-help\" href=\"{doc_prefix}/rustdoc/scraped-examples.html\" target=\"_blank\">?</a>\
Copy link
Member

Choose a reason for hiding this comment

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

I think this is way too much:

  • It's pretty obvious what this is.
  • It'll increase the page size once more for a very minor gain.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

cc @jsha and @camelid who have thoughts on this

Copy link
Member

Choose a reason for hiding this comment

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

I really don't think we should be worrying about a small increase in page size like this. And I don't think it's that obvious what it is. For you, it may be obvious, since you've been immersed in this feature throughout its development, but most users have never even heard of it.

Copy link
Member

Choose a reason for hiding this comment

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

Even if they don't know about the feature, the text itself is pretty explicit: "Examples found in...". The rustdoc book exists and will include a chapter about this but I don't think there is enough interest for it.

Also, it's definitely not a small increase in page size. Especially on big crates with a lot of methods.

Copy link
Member

Choose a reason for hiding this comment

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

The rustdoc book exists and will include a chapter about this but I don't think there is enough interest for it.

I'm not sure what you mean about interest.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we're going to have an explainer in the docs that's distinct from the chapter in the rustdoc book, it should probably just be an overlay / popup like the current help menu. It wouldn't be more than a couple sentences probably.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jsha @GuillaumeGomez can y'all let me know what your final preference is for the scrape examples help message? I'd like to implement it this weekend.

Copy link
Member

Choose a reason for hiding this comment

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

Writing a page and linking to it based on what @jsha suggested.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is it ok if I implement it as a popup though? I really think that would be both a better user experience and simpler for me to implement.

Copy link
Member

Choose a reason for hiding this comment

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

Simpler I'm not sure. Adding an HTML page should be quite easy. Then you just have to link to it. Adding more JS for this doesn't seem the best solution but maybe I'm wrong?

@bors
Copy link
Contributor

bors commented Feb 8, 2022

☔ The latest upstream changes (presumably #93762) made this pull request unmergeable. Please resolve the merge conflicts.

@bors
Copy link
Contributor

bors commented Feb 10, 2022

☔ The latest upstream changes (presumably #93854) made this pull request unmergeable. Please resolve the merge conflicts.

@willcrichton
Copy link
Contributor Author

Alright, well if it's good as is, are there any last requests on this PR? @jsha @GuillaumeGomez @camelid

@willcrichton
Copy link
Contributor Author

@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 Apr 5, 2022
@camelid
Copy link
Member

camelid commented Apr 7, 2022

The effects of this change as listed in the PR description sound good to me; I'll leave reviewing the code to Guillaume.

@camelid camelid removed their assignment Apr 7, 2022

1. For a given item, a maximum of 5 examples are included in the page. The remaining examples are just links to source code.
2. Only one example is shown by default, and the remaining examples are hidden behind a toggle.
3. For a given file that contains examples, only the item containing the examples will be included in the generated documentation.
Copy link
Member

Choose a reason for hiding this comment

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

Can you add a new section at the end providing a link to the rustdoc book chapter talking about this feature please? Something like:

## Official tutorial

If you want more information about this feature, take a look at the [rustdoc book chapter](link here).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just added a commit for this. It looks like:

Screen Shot 2022-04-12 at 11 04 39 AM

@GuillaumeGomez
Copy link
Member

Apart from my comment, looks all good to me!

@willcrichton
Copy link
Contributor Author

Note: I also just added a commit which removes the remaining unwraps in scrape_examples.rs to harden the scraping phase. I would like to make sure that once this feature rolls out, people encounter issues as "my example isn't being scraped" rather than "rustdoc is panicking".

let mut ids = IdMap::default();
format!(
"<div class=\"main-heading\">
"<div class=\"main-heading\">\
Copy link
Member

Choose a reason for hiding this comment

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

Nice!

@GuillaumeGomez
Copy link
Member

All good for me. Then let's go!

@bors: r+

@bors
Copy link
Contributor

bors commented Apr 13, 2022

📌 Commit 6a18b68 has been approved by GuillaumeGomez

@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 Apr 13, 2022
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 13, 2022
Rollup of 6 pull requests

Successful merges:

 - rust-lang#93217 (Improve Rustdoc UI for scraped examples with multiline arguments, fix overflow in line numbers)
 - rust-lang#95885 (Improve error message in case of missing checksum)
 - rust-lang#95962 (Document that DirEntry holds the directory open)
 - rust-lang#95991 (fix: wrong trait import suggestion for T:)
 - rust-lang#96005 (Add missing article to fix "few" to "a few".)
 - rust-lang#96006 (Add a missing article)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit db61452 into rust-lang:master Apr 13, 2022
@rustbot rustbot added this to the 1.62.0 milestone Apr 13, 2022
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.

None yet

8 participants