-
Notifications
You must be signed in to change notification settings - Fork 1.6k
Replaced document exec Command ("copy"); #16604
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
base: master
Are you sure you want to change the base?
Conversation
…ext(str) because it is deprecated.
😊 Welcome! This is either your first contribution to the Istio documentation repo, or
Thanks for contributing! Courtesy of your friendly welcome wagon. |
Hi @gmarav05. Thanks for your PR. I'm waiting for a istio member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
/ok-to-test |
Please now show some evidence of before/after testing everywhere this code it called. Tip: use the Netlify deploy preview link under Checks. |
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.
The issue might seem like it’s just about replacing that particular line, but if you’ve researched the implementation, you’ll notice there are other places in the function where document is used for selection and fallback logic.
When switching to navigator.clipboard.writeText
, you don’t need any of the document-based selection or textarea logic—so the whole function should be refactored, not just that single line
should we refactor that for entire codebase? bro @AritraDey-Dev all tests are passing correctly? |
src/ts/utils.ts
Outdated
@@ -53,7 +53,7 @@ function copyToClipboard(str: string): void { | |||
sel.addRange(oldSelection); | |||
} else { | |||
el.select(); // Select the <textarea> content | |||
document.execCommand("copy"); // Copy - only works as a result of a user action (e.g. click events) | |||
navigator.clipboard.writeText(str); // Copy - only works as a result of a user action (e.g. click events) |
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.
The indenting no longer matches.
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.
Please refactor this function. You don't most of the lines in there in the implementation you did. IE: no longer need to create an element, place the string in that element, then select that element, etc.
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.
@gmarav05,Please read this comment: #16604 (review) — Daniel also mentioned the same concern.You need to remove the unused document
reference from the code, as it’s no longer required if you're using navigator
.
@gmarav05 ? Please do as mentioned |
Replaced deprecated document.execCommand in utils.ts with modern Clipboard.
issue no #16584
Replaced document.execCommand('copy') with navigator.clipboard.writeText(str) for modern, secure clipboard handling.