-
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
Unify run button display with "copy code" button and with mdbook buttons #128394
Conversation
Some changes occurred in HTML/CSS/JS. cc @GuillaumeGomez, @jsha |
This comment has been minimized.
This comment has been minimized.
f607557
to
5b5fd94
Compare
This comment has been minimized.
This comment has been minimized.
This time it seems like I updated all the tests correctly. :D |
Okay, one thing I noticed while poking around the demo that I wish I'd noticed last time: clicking the Copy button does the same thing as clicking the text box itself, so it toggles the visibility lock. It probably shouldn't do that. If you're using a desktop mouse, you hover over the text box, the buttons appear, and you click the Copy button. The buttons are now in "sticky" mode and don't go away, even when the mouse leaves the box. If you're using a touchscreen, you tap the box and the buttons appear, then you click the Copy button and it instantly vanishes, so you never see the confirmation checkmark. |
Oh that's a great point! Fixing that and adding a GUI test for it. |
Done and also updated online demo. |
@rfcbot fcp merge |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
@rfcbot fcp merge |
Team member @notriddle has proposed to merge this. The next step is review by the rest of the tagged team members: No concerns currently listed. 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. |
This comment has been minimized.
This comment has been minimized.
68c2b05
to
27dd6d3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rfcbot reviewed
I'd like us to use the mdBook copy icon too, but this is a very good start in any case. I left comments for changes I'd like on the details.
color: var(--test-arrow-hover-color); | ||
background-color: var(--test-arrow-hover-background-color); | ||
a.test-arrow::before { | ||
content: url('data:image/svg+xml,<svg viewBox="0 0 20 20" width="18" height="20" \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It feels a little large relative to the copy button; what do you think about this? It looks more proportional to my eye. Other code might need to be updated to fix the baseline position too.
content: url('data:image/svg+xml,<svg viewBox="0 0 20 20" width="18" height="20" \ | |
content: url('data:image/svg+xml,<svg viewBox="0 0 20 20" width="15" height="15" \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also would it be better to move this to a CSS variable like the clipboard icon and others?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's only used in one place, so not sure it would be a good idea. Since you suggested to change the image for the copy button, I'll resize when I'll update it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m not sure changing the copy icon is a good idea. mdbook might be using a different one, but crates.io and docs.rs use this one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Then I'll send a PR to mdBook. ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I’m not sure changing the copy icon is a good idea. mdbook might be using a different one, but crates.io and docs.rs use this one.
Good point, I didn't notice that. I do think mdBook's version (which is from Font Awesome) is more common in UIs generally than the rustdoc/crates.io/docs.rs one, but we should definitely be consistent, and it sounds like mdBook is the one out of line here.
Oh, I should clarify that my approval was meant for FCP purposes. I didn't review the code in detail since I think notriddle already did that :) |
🔔 This is now entering its final comment period, as per the review above. 🔔 |
I opened the PR to unify mdBook "copy code" button with rustdoc, docs.rs and crates.io in rust-lang/mdBook#2421. |
☔ The latest upstream changes (presumably #128614) made this pull request unmergeable. Please resolve the merge conflicts. |
27dd6d3
to
59cb159
Compare
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. |
@bors r=t-rustdoc |
…tdoc Unify run button display with "copy code" button and with mdbook buttons Follow-up of rust-lang#128339. It looks like this (coherency++, yeay!): ![Screenshot from 2024-07-30 15-16-31](https://github.com/user-attachments/assets/5e262e5b-f338-4085-94ca-e223033a43db) Can be tested [here](https://rustdoc.crud.net/imperio/run-button/foo/struct.Bar.html). r? `@notriddle`
…llaumeGomez Rollup of 7 pull requests Successful merges: - rust-lang#126245 (Greatly speed up doctests by compiling compatible doctests in one file) - rust-lang#128394 (Unify run button display with "copy code" button and with mdbook buttons) - rust-lang#128878 (Slightly refactor `Flags` in bootstrap) - rust-lang#128886 (Get rid of some `#[allow(rustc::untranslatable_diagnostic)]`) - rust-lang#128929 (Fix codegen-units tests that were disabled 8 years ago) - rust-lang#128967 (std::fs::get_path freebsd update.) - rust-lang#128978 (Use `assert_matches` around the compiler more) r? `@ghost` `@rustbot` modify labels: rollup
…llaumeGomez Rollup of 10 pull requests Successful merges: - rust-lang#128149 (nontemporal_store: make sure that the intrinsic is truly just a hint) - rust-lang#128394 (Unify run button display with "copy code" button and with mdbook buttons) - rust-lang#128537 (const vector passed through to codegen) - rust-lang#128632 (std: do not overwrite style in `get_backtrace_style`) - rust-lang#128878 (Slightly refactor `Flags` in bootstrap) - rust-lang#128886 (Get rid of some `#[allow(rustc::untranslatable_diagnostic)]`) - rust-lang#128929 (Fix codegen-units tests that were disabled 8 years ago) - rust-lang#128937 (Fix warnings in rmake tests on `x86_64-unknown-linux-gnu`) - rust-lang#128978 (Use `assert_matches` around the compiler more) - rust-lang#128994 (Fix bug in `Parser::look_ahead`.) r? `@ghost` `@rustbot` modify labels: rollup
Rollup merge of rust-lang#128394 - GuillaumeGomez:run-button, r=t-rustdoc Unify run button display with "copy code" button and with mdbook buttons Follow-up of rust-lang#128339. It looks like this (coherency++, yeay!): ![Screenshot from 2024-07-30 15-16-31](https://github.com/user-attachments/assets/5e262e5b-f338-4085-94ca-e223033a43db) Can be tested [here](https://rustdoc.crud.net/imperio/run-button/foo/struct.Bar.html). r? `@notriddle`
Follow-up of #128339.
It looks like this (coherency++, yeay!):
Can be tested here.
r? @notriddle