Fix doc [−] button bug by escaping differently #24973
Conversation
The cause of the problem is described here: rust-lang#24797 (comment) . I tested this new `main.js` by changing the `main.js` content of a rendered docs page to this new content. The [−] button worked again.
So that when you click the link, the URL doesn’t get # appended to it. The non-page-global docs toggle link, which is created in `main.js`, already uses this `href` value.
|
(rust_highfive has picked a reviewer for you, use r? to override) |
Error was noted at https://travis-ci.org/rust-lang/rust/builds/60643081#L371 I didn’t just put the content of the node on another line, because that would add spaces around the element content, messing up the JavaScript that checks what the content is.
|
Could this be factored out to MINUS and PLUS vars just because \uwhatever is completely opaque? |
| @@ -1460,7 +1460,8 @@ impl<'a> fmt::Display for Item<'a> { | |||
| try!(write!(fmt, "<span class='out-of-band'>")); | |||
| try!(write!(fmt, | |||
| r##"<span id='render-detail'> | |||
| <a id="toggle-all-docs" href="#" title="collapse all docs">[−]</a> | |||
| <a id="toggle-all-docs" href="javascript:void(0)" | |||
| title="collapse all docs">[−]</a> | |||
There was a problem hiding this comment.
Hmm I suppose this interferes with having a single uniform declaration of this :/
|
Alternatively, this might be a good time to simply replace (my? probably mine...) janky logic with something more robust. One option, I believe, would be to instead mark these with classes and have the rendering specified with CSS's |
|
Proof of concept seems to work: https://jsfiddle.net/gh388tjc/ |
|
A slightly more realistic version of the proof of concept: https://jsfiddle.net/roryokane/669csde1/ I think that approach – just setting the button’s But I agree that it would be nice to consolidate rendering as much as possible. It would also be nice to look at the element’s |
|
There is also an inconsistency in the button rendering that should be fixed. Individual-element buttons surround the inner symbol with The But perhaps all this refactoring should be delayed to another issue. The collapse button is broken right now, and this fix works, and at least doesn’t make the code quality any worse. |
|
I think I'd only really support either a proper solution to the problem or a revert of the problematic commit. |
|
@roryokane would you be ok implementing the class-based approach here? You probably won't be able to use |
|
Sure, I will change the button code to check for a class instead of just checking its text content. |
To separate concerns, instead of checking the state of `#toggle-all-docs` by looking at its label text, I add or remove a class `will-expand` depending on whether the button’s next click will expand everything. (The `if` statement’s two branches were swapped as part of this change.) I moved the desired text values to a function `labelForToggleButton`, so changing the values will be easier. I also note in a comment the other file where the text is duplicated. To allow the labels of both types of toggle buttons to be uniformly set, I added a `span.inner` to the global button too. I split the template in `render.rs` into multiple lines to make room for the `span`, and that adds whitespace around the `[` and `]` text elements. That seems to be okay, though – the page still looks the same. I updated the CSS styling for `.collapse-toggle > .inner` to add a little extra space around the symbol, to make minus signs easier to identify. (`#toggle-all-docs > .inner` does not need the same style, since its text size is bigger, so it naturally puts more space around the symbol.)
|
I have implemented and tested the requested refactoring. Details are in the commit message:
|
This style inconsistency was noted at https://travis-ci.org/rust-lang/rust/builds/61583070#L371
My change in #24797 had a bug, described in that issue’s comments, and first discovered in issue #24918. This fixes it. I tested this new `main.js` by changing the `main.js` content of [a rendered docs page](https://doc.rust-lang.org/std/option/) to this new content. The ‘[−]’ button worked again. I am also including another related fix, because it would require manual merging if I made a separate pull request for it. The page-global ‘[−]’ button currently adds `#` to the end of the URL whenever it is clicked. I am changing its `href` from `#` to `javascript:void(0)` (the same as the `href` for section-specific ‘[−]’ links) to fix that.
|
💔 Test failed - auto-mac-64-nopt-t |
|
@bors: retry On Thu, May 7, 2015 at 11:33 AM, bors [email protected] wrote:
|
|
⚡ Previous build results for auto-mac-64-opt are reusable. Rebuilding only auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-mac-32-opt, auto-mac-64-nopt-t, auto-win-32-nopt-t, auto-win-32-opt, auto-win-64-nopt-t, auto-win-64-opt... |
My change in #24797 had a bug, described in that issue’s comments, and first discovered in issue #24918. This fixes it.
I tested this new
main.jsby changing themain.jscontent of a rendered docs page to this new content. The ‘[−]’ button worked again.I am also including another related fix, because it would require manual merging if I made a separate pull request for it. The page-global ‘[−]’ button currently adds
#to the end of the URL whenever it is clicked. I am changing itshreffrom#tojavascript:void(0)(the same as thehreffor section-specific ‘[−]’ links) to fix that.