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

[rustdoc] Add copy code feature #125779

Merged
merged 4 commits into from
Jul 28, 2024
Merged

Conversation

GuillaumeGomez
Copy link
Member

@GuillaumeGomez GuillaumeGomez commented May 30, 2024

This PR adds a "copy code" to code blocks. Since this is a JS only feature, the HTML is generated with JS when the user hovers the code block to prevent generating DOM unless needed.

Two things to note:

  1. I voluntarily kept the current behaviour of the run button (only when hovering a code block with a mouse) so it doesn't do anything on mobile. I plan to send a follow-up where the buttons would "expandable" or something. Still need to think which approach would be the best.
  2. I used a picture and not text like the run button to remain consistent with the "copy path" button. I'd also prefer for the run button to use a picture (like what is used in mdbook) but again, that's something to be discussed later on.

The rendering looks like this:

Screenshot from 2024-06-03 21-29-48
Screenshot from 2024-06-03 21-30-20

It can be tested here (without the run button) and here (with the run button).

Fixes #86851.

r? @notriddle

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels May 30, 2024
@rustbot
Copy link
Collaborator

rustbot commented May 30, 2024

Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez, @jsha

@rust-log-analyzer

This comment has been minimized.

@notriddle
Copy link
Contributor

The code looked fine to me…

@GuillaumeGomez
Copy link
Member Author

To me as well. 😆 That's why we added such checks. Fixed the warnings :)

@notriddle
Copy link
Contributor

Okay, one thing that I would like to change here.

Can we make the spacing and size of these buttons the same as the ones in the top toolbar? I'm pretty sure the Run button is as huge as it is to try to draw attention to itself, which isn't as big of a deal now that the copy button is there, too.

before

image

after

image

@GuillaumeGomez
Copy link
Member Author

Sounds good to me! It'll also allow me to remove the second clipboard svg.

@GuillaumeGomez
Copy link
Member Author

Updated! I also updated the screenshots and the uploaded docs.

@rust-log-analyzer

This comment has been minimized.

@GuillaumeGomez
Copy link
Member Author

Updated the GUI test I forgot.

@notriddle
Copy link
Contributor

I'd like to tweak the border-radius of the buttons and the code blocks themselves. There's also an inconsistency with the buttons:

  1. The toolbar buttons are 4px apart, but the buttons in the code block are 5px apart.
  2. The copy code button should have a border-radius of 2px, just like the toolbar buttons do.
  3. The copy code button should be 4px inside the code block (right now, it's 5px in).
  4. If the copy code button is 4px into the code box, and has a 2px border-radius, then the code block should have a 6px border-radius. That way, the button and its code block container have border radius circles that align with each other (both have the same center point).

image

@GuillaumeGomez
Copy link
Member Author

Applied all suggestions and updated docs and screenshots.

@notriddle
Copy link
Contributor

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented Jun 3, 2024

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.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Jun 3, 2024
@camelid
Copy link
Member

camelid commented Jun 3, 2024

The button doesn't seem to work on my browser. When I click it my clipboard is unchanged. I'm running Firefox on aarch64 macOS.

@notriddle
Copy link
Contributor

Does the one at the top of the page work? Does the copy button in the docs.rs mega menu work? Does the one in GitHub work?

I'd like to know if it's a bug.

@camelid
Copy link
Member

camelid commented Jun 3, 2024

Yes, the one at the top of the page (to copy the item path) works for me, which I tested on the sample page Guillaume posted that also contains this new feature. It also works for me in GitHub. So does the docs.rs menu. The only place it doesn't work is the buttons generated by this new feature.

@fmease
Copy link
Member

fmease commented Jun 3, 2024

The button doesn't seem to work on my browser. When I click it my clipboard is unchanged

Yes, the one at the top of the page (to copy the item path) works for me, which I tested on the sample page Guillaume posted that also contains this new feature. It also works for me in GitHub. So does the docs.rs menu. The only place it doesn't work is the buttons generated by this new feature.

Same thing happening here! FF 126.0.1, x86_64 Linux. No JS errors getting logged.

@Manishearth
Copy link
Member

@rfcbot concern should-actually-work

let's not land this until it has been thoroughly tested out

Copy link
Member

@fmease fmease left a comment

Choose a reason for hiding this comment

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

I found two major bugs that are blocking. The first one is responsible for "copy code" not working at all (at least for me but likely also for camelid). My review suggestions were manually tested in Firefox and Chromium.

src/librustdoc/html/static/js/main.js Outdated Show resolved Hide resolved
src/librustdoc/html/static/js/main.js Outdated Show resolved Hide resolved
@camelid
Copy link
Member

camelid commented Jul 18, 2024

This looks good, except can you swap the positioning of the copy code and run button? That would bring us into sync with mdBook, which seems preferable to me. (In a future PR, ideally we'd match mdBook's icons as well since they're simpler and less obtrusive.)

@GuillaumeGomez
Copy link
Member Author

In this case I'd rather not: the run button presence is not constant (depends on a setting) whereas the "copy code" button is always there. Since elements are right-aligned, I'd prefer to keep the button always present on the right so it never moves whether or not the "run button" is present.

And yes, in future PR I plan to unify more the buttons with mdBook's.

@Manishearth
Copy link
Member

@rfcbot resolve should-actually-work

@rfcbot rfcbot added the final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. label Jul 18, 2024
@rfcbot
Copy link

rfcbot commented Jul 18, 2024

🔔 This is now entering its final comment period, as per the review above. 🔔

@rfcbot rfcbot removed the proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. label Jul 18, 2024
@camelid
Copy link
Member

camelid commented Jul 18, 2024

In this case I'd rather not: the run button presence is not constant (depends on a setting) whereas the "copy code" button is always there. Since elements are right-aligned, I'd prefer to keep the button always present on the right so it never moves whether or not the "run button" is present.

That's true for mdBook as well; it doesn't always show the run button. I understand your concern, but given that mdBook deals with the same issue, it seems quite unfortunate to have the opposite layout as them for the same features.

@GuillaumeGomez
Copy link
Member Author

Hum... I think in this case that we could improve mdBook. This problem can be seen from two angles: the copy button is always on the left or the opposite: is always at the same position but is on the right. Might be worth re-discussing it when we'll unify buttons more with mdBook. Anyway, sounds like fun times incoming. 😆

@jsha
Copy link
Contributor

jsha commented Jul 18, 2024

I can't publish it, I think the website is down for now. cc @jsha

Fixed! Sorry about that.

@GuillaumeGomez
Copy link
Member Author

Awesome, thanks!

@rfcbot rfcbot added finished-final-comment-period The final comment period is finished for this PR / Issue. and removed final-comment-period In the final comment period and will be merged soon unless new substantive objections are raised. labels Jul 28, 2024
@rfcbot
Copy link

rfcbot commented Jul 28, 2024

The final comment period, with a disposition to merge, as per the review above, is now complete.

As the automated representative of the governance process, I would like to thank the author for their work and everyone else who contributed.

This will be merged soon.

@rfcbot rfcbot added the to-announce Announce this issue on triage meeting label Jul 28, 2024
@GuillaumeGomez
Copy link
Member Author

@bors r=rustdoc-team

@bors
Copy link
Contributor

bors commented Jul 28, 2024

📌 Commit 26d7251 has been approved by rustdoc-team

It is now in the queue for this repository.

@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 Jul 28, 2024
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Jul 28, 2024
…c-team

[rustdoc] Add copy code feature

This PR adds a "copy code" to code blocks. Since this is a JS only feature, the HTML is generated with JS when the user hovers the code block to prevent generating DOM unless needed.

Two things to note:
 1. I voluntarily kept the current behaviour of the run button (only when hovering a code block with a mouse) so it doesn't do anything on mobile. I plan to send a follow-up where the buttons would "expandable" or something. Still need to think which approach would be the best.
 2. I used a picture and not text like the run button to remain consistent with the "copy path" button. I'd also prefer for the run button to use a picture (like what is used in mdbook) but again, that's something to be discussed later on.

The rendering looks like this:

![Screenshot from 2024-06-03 21-29-48](https://github.com/rust-lang/rust/assets/3050060/a0b18f9c-b3dd-4a65-89a7-5a7a303b5c2b)
![Screenshot from 2024-06-03 21-30-20](https://github.com/rust-lang/rust/assets/3050060/b3b084ff-2716-4160-820b-d4774681a961)

It can be tested [here](https://guillaume-gomez.fr/rustdoc/bar/struct.Bar.html) (without the run button) and [here](https://guillaume-gomez.fr/rustdoc/foo/struct.Bar.html) (with the run button).

Fixes rust-lang#86851.

r? `@notriddle`
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 28, 2024
…llaumeGomez

Rollup of 6 pull requests

Successful merges:

 - rust-lang#125779 ([rustdoc] Add copy code feature)
 - rust-lang#127765 (Fix doc nits)
 - rust-lang#127860 (deps: dedup object, wasmparser, wasm-encoder)
 - rust-lang#128103 (add `is_multiple_of` for unsigned integer types)
 - rust-lang#128228 (Stabilize `const_waker`)
 - rust-lang#128240 (Add links from `assert_eq!` docs to `debug_assert_eq!`, etc.)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit a8cc24a into rust-lang:master Jul 28, 2024
6 checks passed
@rustbot rustbot added this to the 1.82.0 milestone Jul 28, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jul 28, 2024
Rollup merge of rust-lang#125779 - GuillaumeGomez:copy-code, r=rustdoc-team

[rustdoc] Add copy code feature

This PR adds a "copy code" to code blocks. Since this is a JS only feature, the HTML is generated with JS when the user hovers the code block to prevent generating DOM unless needed.

Two things to note:
 1. I voluntarily kept the current behaviour of the run button (only when hovering a code block with a mouse) so it doesn't do anything on mobile. I plan to send a follow-up where the buttons would "expandable" or something. Still need to think which approach would be the best.
 2. I used a picture and not text like the run button to remain consistent with the "copy path" button. I'd also prefer for the run button to use a picture (like what is used in mdbook) but again, that's something to be discussed later on.

The rendering looks like this:

![Screenshot from 2024-06-03 21-29-48](https://github.com/rust-lang/rust/assets/3050060/a0b18f9c-b3dd-4a65-89a7-5a7a303b5c2b)
![Screenshot from 2024-06-03 21-30-20](https://github.com/rust-lang/rust/assets/3050060/b3b084ff-2716-4160-820b-d4774681a961)

It can be tested [here](https://guillaume-gomez.fr/rustdoc/bar/struct.Bar.html) (without the run button) and [here](https://guillaume-gomez.fr/rustdoc/foo/struct.Bar.html) (with the run button).

Fixes rust-lang#86851.

r? ``@notriddle``
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Jul 30, 2024
… r=notriddle

[rustdoc] Make the buttons remain when code example is clicked

Follow-up of rust-lang#125779.

One current issue we have with "run" button and the newly added copy code button is that if you're on mobile devices, you can't use them. I took a look at how `mdbook` is handling it and when you click on a code example, they show the buttons. I think it's a really good idea as if you want to copy the code on your mobile device, you will click on it, showing the buttons.

Feature can be tested [here](https://rustdoc.crud.net/imperio/click-code-example/foo/struct.Bar.html).

r? `@notriddle`
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jul 30, 2024
Rollup merge of rust-lang#128339 - GuillaumeGomez:click-code-example, r=notriddle

[rustdoc] Make the buttons remain when code example is clicked

Follow-up of rust-lang#125779.

One current issue we have with "run" button and the newly added copy code button is that if you're on mobile devices, you can't use them. I took a look at how `mdbook` is handling it and when you click on a code example, they show the buttons. I think it's a really good idea as if you want to copy the code on your mobile device, you will click on it, showing the buttons.

Feature can be tested [here](https://rustdoc.crud.net/imperio/click-code-example/foo/struct.Bar.html).

r? `@notriddle`
@apiraino apiraino removed the to-announce Announce this issue on triage meeting label Aug 1, 2024
@GuillaumeGomez GuillaumeGomez deleted the copy-code branch August 19, 2024 12:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. finished-final-comment-period The final comment period is finished for this PR / Issue. 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.

Add copy button for code blocks