Reset the docs' copy path button after 1 second #84740
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Some changes occurred in HTML/CSS/JS. |
|
r? @CraftSpider (rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
jyn514
added
T-rustdoc
wooster0
commented
Apr 30, 2021
src/librustdoc/html/static/main.js
Outdated
| @@ -1513,4 +1515,12 @@ function copy_path(but) { | |||
| document.body.removeChild(el); | |||
|
|
|||
| but.textContent = '✓'; | |||
|
|
|||
| window.clearTimeout(reset_button_timeout); | |||
There was a problem hiding this comment.
This won't error the first time: https://developer.mozilla.org/en-US/docs/Web/API/WindowOrWorkerGlobalScope/clearTimeout#notes.
This comment has been minimized.
This comment has been minimized.
|
In that failed build log I can see:
Maybe uninitialized declarations like |
|
|
src/librustdoc/html/static/main.js
Outdated
| @@ -1490,6 +1490,8 @@ function hideThemeButtonState() { | |||
| searchState.setup(); | |||
| }()); | |||
|
|
|||
| let reset_button_timeout; | |||
There was a problem hiding this comment.
I really don't like global variables like this. Instead, please wrap copy_path into an anonymous function like we do in multiple places here and move the reset_button_timeout declaration there.
Another thing: even though it's valid to send an undefined variable to clearTimeout, please initialize your value to null and check it before calling clearTimeout.
src/librustdoc/html/static/main.js
Outdated
| window.clearTimeout(reset_button_timeout); | ||
|
|
||
| function reset_button() { | ||
| but.textContent = '⎘'; |
There was a problem hiding this comment.
Please reset reset_button_timeout to null in here.
src/librustdoc/html/static/main.js
Outdated
| path.push(child.textContent); | ||
| } | ||
| }); | ||
| function copy_path(but) { |
There was a problem hiding this comment.
You need to put this function into window, otherwise the onclick event won't find it.
window.copy_path = copy_path;Or you can declare it directly like this:
Suggested change
| function copy_path(but) { | |
| window.copy_path = function(but) { |
(and don't forget the ; at the end ;) )
GuillaumeGomez
approved these changes
Apr 30, 2021
|
Thanks! @bors: r+ rollup |
|
📌 Commit bea99a5 has been approved by |
bors
added
S-waiting-on-bors
GuillaumeGomez
added a commit
to GuillaumeGomez/rust
that referenced
this pull request
Apr 30, 2021
Reset the docs' copy path button after 1 second I like that this copy path button on the top next to the type/module's name changes to a check mark when you successfully clicked and copied the path but I find it really weird how the icon stays that check mark forever after the first time of clicking it. Imagine you leave that documentation tab open and come back after 2 hours and you still see that check mark in that box because you copied the path 2 hours ago. You will probably be confused and you might've forgotten what that button even does (even more so currently where this is a new feature, or when you simply don't use it often), so I really think at some point it should go back to the ⎘ icon which, at least to me, pretty clearly indicates copying, whereas the check mark (if it stays there for so long) could falsely look like a verification mark indicating "this module is verified" or something like that. I believe after a longer period of time it's not logical to still tell the user "yes you've copied this successful". In addition to this timeout, maybe it could be made so that you can't copy again until this cooldown of 1 second is over, but I'm not sure how useful or user-friendly that feature would be so maybe it's fine the way it is now. Also the timeout is cleared every time you click again so if you constantly click it, it won't reset during that.
JohnTitor
added a commit
to JohnTitor/rust
that referenced
this pull request
May 1, 2021
Reset the docs' copy path button after 1 second I like that this copy path button on the top next to the type/module's name changes to a check mark when you successfully clicked and copied the path but I find it really weird how the icon stays that check mark forever after the first time of clicking it. Imagine you leave that documentation tab open and come back after 2 hours and you still see that check mark in that box because you copied the path 2 hours ago. You will probably be confused and you might've forgotten what that button even does (even more so currently where this is a new feature, or when you simply don't use it often), so I really think at some point it should go back to the ⎘ icon which, at least to me, pretty clearly indicates copying, whereas the check mark (if it stays there for so long) could falsely look like a verification mark indicating "this module is verified" or something like that. I believe after a longer period of time it's not logical to still tell the user "yes you've copied this successful". In addition to this timeout, maybe it could be made so that you can't copy again until this cooldown of 1 second is over, but I'm not sure how useful or user-friendly that feature would be so maybe it's fine the way it is now. Also the timeout is cleared every time you click again so if you constantly click it, it won't reset during that.
This was referenced May 1, 2021
bors
added a commit
to rust-lang-ci/rust
that referenced
this pull request
May 1, 2021
Rollup of 8 pull requests Successful merges: - rust-lang#84601 (rustdoc: Only store locations in Cache::extern_locations and calculate the other info on-demand) - rust-lang#84704 (platform-support.md: Update for consistency with Target Tier Policy) - rust-lang#84724 (Replace llvm::sys::fs::F_None with llvm::sys::fs::OF_None) - rust-lang#84740 (Reset the docs' copy path button after 1 second) - rust-lang#84749 (Sync `rustc_codegen_cranelift`) - rust-lang#84756 (Add a ToC to the Target Tier Policy documentation) - rust-lang#84765 (Update cargo) - rust-lang#84774 (Fix misspelling) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I like that this copy path button on the top next to the type/module's name changes to a check mark when you successfully clicked and copied the path but I find it really weird how the icon stays that check mark forever after the first time of clicking it. Imagine you leave that documentation tab open and come back after 2 hours and you still see that check mark in that box because you copied the path 2 hours ago. You will probably be confused and you might've forgotten what that button even does (even more so currently where this is a new feature, or when you simply don't use it often), so I really think at some point it should go back to the ⎘ icon which, at least to me, pretty clearly indicates copying, whereas the check mark (if it stays there for so long) could falsely look like a verification mark indicating "this module is verified" or something like that.
I believe after a longer period of time it's not logical to still tell the user "yes you've copied this successful".
In addition to this timeout, maybe it could be made so that you can't copy again until this cooldown of 1 second is over, but I'm not sure how useful or user-friendly that feature would be so maybe it's fine the way it is now.
Also the timeout is cleared every time you click again so if you constantly click it, it won't reset during that.