-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Add "copy to clipboard" for all code blocks and "expand" buttons #86892
Add "copy to clipboard" for all code blocks and "expand" buttons #86892
Conversation
Some changes occurred in HTML/CSS themes. Some changes occurred in HTML/CSS/JS. A change occurred in the Ayu theme. cc @Cldfire |
I don't have time to review this right now. Is jsha still on vacation? They'd be a better reviewer. |
They are in vacation indeed. I thought you were back, my bad. Feel free to assign someone else then. :) |
r? @Manishearth |
The "expand" button looks to me like it would put the code in full-screen mode. mdBook already has a feature like this, so if we are going to implement it as well, it would probably be best to be consistent with mdBook's icon. Theirs is an eye icon (link): |
@camelid Oh good idea! I'm gonna "steal" it from them. :3 |
bc5a059
to
cef0964
Compare
I'd prefer the expanded code to be at least a little bit greyed out or something |
TBH I'd rather wait for @jsha to be back to give feedback on the UI. |
cef0964
to
4dcc8b1
Compare
Just my opinion, but it looks kinda ugly to me that the Play button is so big and different from the other twos. |
It's part of the things we need @jsha for. ;) |
This comment has been minimized.
This comment has been minimized.
4dcc8b1
to
8790a87
Compare
While we're at it, could this be changed to use the play ▶ button for running the sample, like how mdbook does it? |
Well, why not. 😆 I'll also hide the buttons by default then. |
This comment has been minimized.
This comment has been minimized.
8790a87
to
3ad9680
Compare
This comment has been minimized.
This comment has been minimized.
r? @notriddle - do you have time to review this? No worries if not, I can ask someone else :) |
8aff4e1
to
78a3276
Compare
@GuillaumeGomez can you add instructions in the PR description on how to test locally? I've checked out the branch locally and run |
@jsha I did nothing in particular, just built the std docs by doing Just search for |
Thanks! So, one piece of quick UI feedback: Right now we have one button that uses text ("Run") and two that are icons. We should use either all text or all icons in this location. My general preference is text over icons, but given there are three buttons, Run / Reveal / Copy would probably be too big, so we should make Run into a "play" icon like in mdBook. Though, one other thought just to put it out there: We could change this to a small menu, like "tools", which would have Run / Reveal / Copy as menu items below it. By the way, in #86363 there was some confusion about why we don't have "Run" on all documentation. I've felt that confusion myself a number of times. I think that's because the Run button disappears when it's not available, rather than being greyed out. If we grey out the Run button on non-std crates, that allows us to add a tooltip that says "Rust playground not available for this crate" or similarly explains the problem. The Reveal button will only be present on a small subset of examples. Given that, it feels awkward to stick it in the middle of the two other icons. I have a slight preference for putting it all the way to the right, so the relative position of Run and Copy is always the same. |
So, we have the following propositions:
Anyway, which one seems preferable to you @jsha and to the others @rust-lang/rustdoc ? |
I think 3 icons are small enough that a menu is unnecessary, and I'd prefer avoiding any hover states (partly selfishly, because I use vi-mode browser nav). Making the run button appear greyed out could actually be great for getting crates to enable support for it, if the text was something like "Rust playground not enabled for this crate" the crates that are lucky enough to be included in the playground might start adding |
Right now I think this is the preferable option. Note: I'm saying the "eye" (reveal) button should be on the right, not the "run" button.
I'm also generally anti-hover for menus. In rust-lang/docs.rs#1081 I removed some hover menus from docs.rs, and linked to https://uxmovement.com/navigation/why-hover-menus-do-users-more-harm-than-good/ for details about why. |
I also prefer avoiding hover menus Though a thought: The eye button currently displays the state that one will reach by clicking it, not the current state. I know that UI design tends to be splti on this: i kinda prefer when it shows the current state, so it shows an eye when stuff is visible, and a crossed eye when stuff is hidden. |
I'm not picky about which option it goes with, as long as its the same one as The Book (which is rendered using mdBook). Which means it should be showing a crossed eye when stuff is visible, and a plain eye when stuff is hidden. |
I agree on "same as The Book." Another useful question: are there other places where we should "current state" vs "state to be transitioned to" on a clickable thing? One example would be the |
Personally, I would prefer we don't add these new buttons at all. Rustdoc's UI already has a lot of stuff going on, and having buttons on every code example adds even more visual components to the actual text of the docs. As I mentioned in #86851 (comment), I don't feel like I understand what the use case of this feature is. Part of why I don't understand the use case is that I find that most code examples I look at can't just be copied and pasted into my code; only a small piece, say a function call, is relevant, and that I can type myself. For example, with As discussed, mdBook also has these features, but, at least in mdBooks like The Book, the code samples are part of larger programs that the reader is building up, so it's more useful to have copy-to-clipboard in that situation. Whereas with rustdoc, code examples are usually not "real" programs but just demonstrations of the API in context. I also want to note that a small technical downside to this feature is that it will increase page sizes, because now we need to include the hidden parts of examples in the page too. But, if the consensus is to add this feature, I think I would lean towards having a three-vertical-dots–style menu, which, when clicked/tapped (not hovered), would show a list:
I think having a menu opened by tapping an icon instead of three individual, potentially named, buttons would be less intrusive on the regular docs reading experience. |
FWIW I am very ambivalent about adding this feature; I kinda agree with @camelid's position. But I'm not going to block it if most of the rest of the team wants it |
☔ The latest upstream changes (presumably #89386) made this pull request unmergeable. Please resolve the merge conflicts. |
Originally it was mostly about showing the hidden code in case the run button isn't available because people complained that it was fairly complicated for beginners to write the wrapping code (when |
I think we should find a way to address that specific problem rather than a button to copy the whole code including hidden lines, that seems even more confusing. |
In general I think we should be filing issues for problems, not solutions, and working from there. |
Which is why I was suggesting to remove the add of the copy button and only keep the show/hide one. |
I'm going to close this for now untill we decide on the proper improvement; I don't think it will be exactly this PR. |
Fixes #86851.
A little demo of the expand/collapse part:
Peek.2021-07-07.17-02.mp4
Now some technical details: to prevent the copy of an
img
tag, I moved the image in the background of the button instead, allowing me to move it inside the CSS, preventing the size of the HTML to grow too much. By doing so however, it forced me to modifyrustdoc.css
in rustdoc directly when we generate the file. Nothing super complicated but still important enough to be noted.The generated hidden lines are generated inside a
<span class="hidden">
, the JS handles the rest, it keeps the syntax highlighting of course. It forced me to update thewrite_code
but nothing fancy either.Another important detail: the "expand/collapse" button isn't generated if there is no hidden content.
All the new buttons are hidden in case the JS is disabled (
noscript.js
).For the "copy code" feature, unfortunately, it cannot be tested (or at least I didn't find out how). So the only thing I can recommend is to try out locally. It's supposed to work as follow: only the visible code is copied. As simple as that. :)
Last information: I originally intended to hide the code blocks buttons (even the "run" one) by default and only make them appear when hovering the code block but I'll do that in another PR because I think this one is already big enough...
EDIT: Last information bis: In the JS code, the functions
copyCode
andcopy_path
are very similar, I intend to merge them even further in a later PR (I already started by creating thecopyContentToClipboard
function).cc @jsha @Manishearth
r? @jyn514