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.

Sign up for GitHub

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

rustdoc: Foldable impl blocks #47894

Merged

bors merged 12 commits into rust-lang:master from vi:rustdoc_foldable_impls

Feb 28, 2018

Contributor

vi commented Jan 31, 2018 •

edited

Loading

Addresses #40363, #45720, #24483, #23986 and so on

Expands and refactors collapseDocs and toggleAllDocs
Adds [-] toggle to all impls (including inherent impl)
Makes it hiding though main css file, not though element inline style

May need to be addressed:

"[-]" and anchor link copier are overlaid a bit
Inherent methods are also hidden by the global [-] toggle.
Auto-collapsing "Iterator" and so on by default is not implemented yet
Tested only shallowly and only in Chromiuim
No tests. Are there tests for css/js part here?
The new implementation may be a bit slower.

What next steps are need to be done before the integration?


rustdoc: Foldable impl blocks

73f52d3

Addresses rust-lang#40363, rust-lang#45720, rust-lang#24483, rust-lang#23986 and so on

* Expands and refactors collapseDocs and toggleAllDocs
* Adds [-] toggle to all impls (including inherent impl)
* Makes it hiding though main css file, not though element style

May need to be addressed:

* "[-]" and anchor link copier are overlaid a bit
* Inherent methods are also hidden by the global [-] toggle.
* Auto-collapsing "Iterator" and so on by default is not implemented yet
* Tested only shallowly and only in Chromiuim
* No tests. Are there tests for css/js part here?
* The new implementation may be a bit slower.

rust-highfive assigned GuillaumeGomez

Collaborator

rust-highfive commented Jan 31, 2018

Some changes occurred in HTML/CSS.

cc @GuillaumeGomez

Collaborator

rust-highfive commented Jan 31, 2018

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) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.


rustdoc: Fix tidy errors found by travis

7cd0280

kennytm added A-docs S-waiting-on-review labels

Member

kennytm commented Jan 31, 2018

Thanks for the PR! We’ll periodically check in on it to make sure that @GuillaumeGomez or someone else from the team reviews it soon.

GuillaumeGomez reviewed

View reviewed changes

src/librustdoc/html/static/main.js Outdated

+ }
+ }
+ if(action == "show") {

Member

GuillaumeGomez Jan 31, 2018

Missing whitespace after if.

GuillaumeGomez reviewed

View reviewed changes

src/librustdoc/html/static/main.js Outdated

+ var action = mode;
+ if (action == "toggle") {
+ if(hasClass(relatedDoc, "fns-now-collapsed")) {

Member

GuillaumeGomez Jan 31, 2018

Missing whitespace after if.

GuillaumeGomez reviewed

View reviewed changes

src/librustdoc/html/static/main.js Outdated

+ relatedDoc = relatedDoc.nextElementSibling;
+ }
+ if (!relatedDoc) return;

Member

GuillaumeGomez Jan 31, 2018

Please don't do that. Use {}, it makes reading easier.

GuillaumeGomez reviewed

View reviewed changes

src/librustdoc/html/static/main.js Outdated

+ removeClass(n, "hidden-by-impl-hider");
+ }
+ var ns = n.nextElementSibling;
+ while(true) {

Member

GuillaumeGomez Jan 31, 2018

Missing whitespace after while.

GuillaumeGomez reviewed

View reviewed changes

src/librustdoc/html/static/main.js Outdated

+ // we are collapsing the impl block
+ function implHider(addOrRemove) {
+ return function(n) {
+ if(hasClass(n, "method")) {

Member

GuillaumeGomez Jan 31, 2018

Missing whitespace after if.

GuillaumeGomez reviewed

View reviewed changes

src/librustdoc/html/static/main.js Outdated

+ if (hasClass(relatedDoc, "docblock")) {
+ var action = mode;
+ if (action == "toggle") {
+ if(hasClass(relatedDoc, "hidden-by-usual-hider")) {

Member

GuillaumeGomez Jan 31, 2018

Missing whitespace after if.

GuillaumeGomez reviewed

View reviewed changes

src/librustdoc/html/static/main.js Outdated

+ }
+ }
+ if(action == "show") {

Member

GuillaumeGomez Jan 31, 2018

Also, please use ===.

GuillaumeGomez reviewed

View reviewed changes

src/librustdoc/html/static/main.js Outdated

+ var action = mode;
+ if (action == "toggle") {
+ if(hasClass(relatedDoc, "fns-now-collapsed")) {
+ action="show";

Member

GuillaumeGomez Jan 31, 2018

Missing whitespace around =.

GuillaumeGomez reviewed

View reviewed changes

src/librustdoc/html/static/main.js Outdated

+ if(hasClass(relatedDoc, "fns-now-collapsed")) {
+ action="show";
+ } else {
+ action="hide";

Member

GuillaumeGomez Jan 31, 2018

Missing whitespace around =.

GuillaumeGomez reviewed

View reviewed changes

src/librustdoc/html/static/main.js Outdated

+ removeClass(relatedDoc, "fns-now-collapsed");
+ onEach(toggle.childNodes, adjustToggle(false));
+ onEach(relatedDoc.childNodes, implHider(false));
+ } else if (action == "hide") {

Member

GuillaumeGomez Jan 31, 2018

Please use ===.

GuillaumeGomez reviewed

View reviewed changes

src/librustdoc/html/static/themes/dark.css Outdated

@@ @@ -386,3 +386,8 @@ kbd { @@
  background: #353535;
  }
 }
+.hidden-by-impl-hider,

Member

GuillaumeGomez Jan 31, 2018

This rule has nothing to do in themes. Please put in rustdoc.css at the upper level.

GuillaumeGomez reviewed

View reviewed changes

src/librustdoc/html/static/themes/main.css Outdated

@@ @@ -383,3 +383,8 @@ kbd { @@
  background: #fff;
  }
 }
+.hidden-by-impl-hider,

Member

GuillaumeGomez Jan 31, 2018

This rule has nothing to do in themes. Please put in rustdoc.css at the upper level.

Member

GuillaumeGomez commented Jan 31, 2018

I made a first quick review. I'll test what it's doing this evening or tomorrow. Seems globally good otherwise (just nits mainly) .

kennytm added S-waiting-on-author and removed S-waiting-on-review labels

Contributor Author

vi commented Jan 31, 2018

Is there maybe some automatic script that makes whitespace in javascript files meet the standard in one move?

Contributor Author

vi commented Jan 31, 2018

Where the list of collapsed-by-default traits should reside? In Rust part (marking html elements with a class like "collapsed-by-default") or in Javascript part?


rustdoc: Fix some minor issues per reviewer

c904b1b

kennytm added S-waiting-on-review and removed S-waiting-on-author labels

Contributor

Thomasdezeeuw commented Jan 31, 2018

@vi Why not collapse all trait implementations by default? In most cases you want to visit the actual trait documentation anyway, since the documentation at the implementation only shows a single line or so per method.

Contributor Author

vi commented Jan 31, 2018

A good idea. Important things like associated types/consts or impl-level documentation are visible even when collapsed anyway.

Contributor

bors commented Feb 24, 2018

💔 Test failed - status-travis

bors added S-waiting-on-review and removed S-waiting-on-bors labels

Member

kennytm commented Feb 24, 2018

@bors retry

3 hour timeout in dist-{powerpc,powerpc64le}-linux.

bors added S-waiting-on-bors and removed S-waiting-on-review labels

Manishearth mentioned this pull request

Rollup of 9 pull requests #48526

Closed

Manishearth added a commit to Manishearth/rust that referenced this pull request


Rollup merge of rust-lang#47894 - vi:rustdoc_foldable_impls, r=Guilla…

1b03ac6

…umeGomez,QuietMisdreavus

rustdoc: Foldable impl blocks

Addresses rust-lang#40363, rust-lang#45720, rust-lang#24483, rust-lang#23986 and so on

* Expands and refactors collapseDocs and toggleAllDocs
* Adds [-] toggle to all impls (including inherent impl)
* Makes it hiding though main css file, not though element inline style

May need to be addressed:

* "[-]" and anchor link copier are overlaid a bit
* Inherent methods are also hidden by the global [-] toggle.
* Auto-collapsing "Iterator" and so on by default is not implemented yet
* Tested only shallowly and only in Chromiuim
* No tests. Are there tests for css/js part here?
* The new implementation may be a bit slower.

What next steps are need to be done before the integration?

Contributor

bors commented Feb 26, 2018

⌛ Testing commit df1b9a8 with merge 2452e2bfc1b92af2cb68e162a3cad0f9d473f177...

Contributor

bors commented Feb 26, 2018

💔 Test failed - status-travis

bors added S-waiting-on-review and removed S-waiting-on-bors labels

Member

kennytm commented Feb 26, 2018

@bors retry

3 hour timeout in dist-s390x-linux.

Timing breakdown

make-prepare                	  47.14
pytest/bootstrap            	   0.00
stage0-std                  	  63.00
stage0-test                 	  17.40
stage0-rustc                	1118.22
llvm                        	 302.07
stage0-trans-llvm           	 166.90
stage1-std                  	  62.31
stage1-test                 	  17.31
stage1-rustc                	1173.68
stage1-trans-llvm           	 181.68
stage1-std                  	  67.91
stage0-unstable-book-gen    	  10.28
stage0-rustbook             	 167.91
stage2-rustdoc              	 165.16
doc/std                     	  38.67
doc/compiler                	  16.36
stage1-rustc                	1177.74
doc/proc_macro              	  27.52
stage2-error_index_generator	   5.34
stage0-fabricate            	  90.44
llvm                        	2465.59
stage1-trans-llvm           	 189.50
stage2-rustdoc              	 160.93
stage2-cargo                	 401.38
stage2-rustfmt              	 240.86
stage2-cargo-fmt            	   0.00

41 minutes spent on compiling LLVM (for s390x), maybe sccache network error?

bors added S-waiting-on-bors and removed S-waiting-on-review labels

Contributor

bors commented Feb 28, 2018

⌛ Testing commit df1b9a8 with merge 89e5a07...

bors added a commit that referenced this pull request


Auto merge of #47894 - vi:rustdoc_foldable_impls, r=GuillaumeGomez,Qu…

89e5a07

…ietMisdreavus

rustdoc: Foldable impl blocks

Addresses #40363, #45720, #24483, #23986 and so on

* Expands and refactors collapseDocs and toggleAllDocs
* Adds [-] toggle to all impls (including inherent impl)
* Makes it hiding though main css file, not though element inline style

May need to be addressed:

* "[-]" and anchor link copier are overlaid a bit
* Inherent methods are also hidden by the global [-] toggle.
* Auto-collapsing "Iterator" and so on by default is not implemented yet
* Tested only shallowly and only in Chromiuim
* No tests. Are there tests for css/js part here?
* The new implementation may be a bit slower.

What next steps are need to be done before the integration?

Contributor

bors commented Feb 28, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: GuillaumeGomez,QuietMisdreavus
Pushing 89e5a07 to master...

bors merged commit df1b9a8 into rust-lang:master

Contributor Author

vi commented Mar 15, 2018

Looks like § started to intersect unrelated headings (at least on docs.rs):

Maybe a follow-up pull request is needed. Or shall entire § handling be overhauled? For example, it does not allow to get an anchored link to a particular function inside impl.

Member

shepmaster commented Mar 15, 2018

Or shall entire § handling be overhauled?

I often find the § disappearing as I move the cursor from the header to the § in order to copy the link, so I'd be in favor of fixing that ;-).

Member

GuillaumeGomez commented Mar 16, 2018

It's been fixed in rustdoc already.

mbrubeck mentioned this pull request

rustdoc trait implementations should be collapsible #24483

Closed

jsha mentioned this pull request

rustdoc: make trait methods findable with Ctrl-F #85923

Closed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

A-docs S-waiting-on-bors