Skip to content

[Console] Fix copy as cURL#97968

Merged
jloleysens merged 3 commits intoelastic:masterfrom
jloleysens:console/fix-copy-as-curl
Apr 23, 2021
Merged

[Console] Fix copy as cURL#97968
jloleysens merged 3 commits intoelastic:masterfrom
jloleysens:console/fix-copy-as-curl

Conversation

@jloleysens
Copy link
Copy Markdown
Contributor

@jloleysens jloleysens commented Apr 22, 2021 •
edited
Loading

Summary

Fix #97828

Updated copy as cURL functionality to rather use the Clipboard API via navigator.clipboard.

Release note

We fixed the "Copy as cURL" functionality in Console which was not copying to the system clipboard on newer browser versions.

Checklist

  • Test on Chrome, Firefox, Safari

@jloleysens jloleysens added Feature:Console Dev Tools Console Feature Feature:Dev Tools release_note:fix v8.0.0 Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more t// v7.14.0 v7.13.0 v7.12.2 labels Apr 22, 2021
@jloleysens jloleysens requested a review from sebelga April 22, 2021 09:32
@jloleysens jloleysens requested a review from a team as a code owner April 22, 2021 09:32
@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Apr 22, 2021

Pinging @elastic/es-ui (Team:Elasticsearch UI)

@jloleysens jloleysens requested review from yuliacech and removed request for sebelga April 22, 2021 09:36
yuliacech
yuliacech reviewed Apr 22, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@yuliacech yuliacech left a comment

Choose a reason for hiding this comment

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

Hi @jloleysens, thanks a lot for fixing this bug!
Since execCommand is already deprecated and supported browsers are evergreen, I would suggest removing the legacy function and instead notify the user with an error popup or similar if the clipboard copy failed, WDYT?

@jloleysens jloleysens changed the title [Console] Fix copy as cUrl [Console] Fix copy as cURL Apr 22, 2021
@jloleysens
Copy link
Copy Markdown
Contributor Author

jloleysens commented Apr 22, 2021

@yuliacech thanks for the review! I agree with your suggestion. Especially if we guard this button with a check for the presence of the system "clipboard" as the previous was guarded by the copy command check.

Would you mind taking another look?

yuliacech
yuliacech approved these changes Apr 22, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@yuliacech yuliacech left a comment

Choose a reason for hiding this comment

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

Thanks @jloleysens , code changes LGTM! 👍
It doesn't look like we have tests for this plugin though, right?

@kibanamachine
Copy link
Copy Markdown
Contributor

kibanamachine commented Apr 22, 2021

💚 Build Succeeded

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
console 890.4KB 890.7KB +294.0B

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@jloleysens jloleysens merged commit 851ee69 into elastic:master Apr 23, 2021
@jloleysens jloleysens deleted the console/fix-copy-as-curl branch April 23, 2021 15:18
@jloleysens
Copy link
Copy Markdown
Contributor Author

jloleysens commented Apr 23, 2021

It doesn't look like we have tests for this plugin though, right?

We do have some CIT coverage here: src/plugins/console/public/application/containers/editor/legacy/console_editor/editor.test.tsx. But I think we will need to add some more in future. The code inside of the src/plugins/console/public/lib does have some amount of coverage. But it is not standardised (i.e., there is no __jest__/client_integration folder).

I'll open an issue to capture this work!

@jloleysens
Copy link
Copy Markdown
Contributor Author

jloleysens commented Apr 23, 2021

Opened: #98158

@jloleysens jloleysens mentioned this pull request Apr 23, 2021
Merged
jloleysens added a commit to jloleysens/kibana that referenced this pull request Apr 23, 2021
@jloleysens
[Console] Fix copy as cURL (elastic#97968)
2bce89d 
* updated code to use Clipboard API rather than document.execCommand

* added link to deprecation of document.execCommand

* removed legacy copy text method
@jloleysens jloleysens mentioned this pull request Apr 23, 2021
Merged
jloleysens added a commit to jloleysens/kibana that referenced this pull request Apr 23, 2021
@jloleysens
[Console] Fix copy as cURL (elastic#97968)
79d5190 
* updated code to use Clipboard API rather than document.execCommand

* added link to deprecation of document.execCommand

* removed legacy copy text method
@jloleysens jloleysens mentioned this pull request Apr 23, 2021
Merged
jloleysens added a commit to jloleysens/kibana that referenced this pull request Apr 23, 2021
@jloleysens
[Console] Fix copy as cURL (elastic#97968)
71b401e 
* updated code to use Clipboard API rather than document.execCommand

* added link to deprecation of document.execCommand

* removed legacy copy text method
jloleysens added a commit that referenced this pull request Apr 23, 2021
@jloleysens
[Console] Fix copy as cURL (#97968) (#98160)
65768c3 
* updated code to use Clipboard API rather than document.execCommand

* added link to deprecation of document.execCommand

* removed legacy copy text method
jloleysens added a commit that referenced this pull request Apr 23, 2021
@jloleysens
[Console] Fix copy as cURL (#97968) (#98163)
a30eea1 
* updated code to use Clipboard API rather than document.execCommand

* added link to deprecation of document.execCommand

* removed legacy copy text method
cqliu1 pushed a commit to cqliu1/kibana that referenced this pull request Apr 23, 2021
@jloleysens
[Console] Fix copy as cURL (elastic#97968) (elastic#98161)
35d2ef4 
* updated code to use Clipboard API rather than document.execCommand

* added link to deprecation of document.execCommand

* removed legacy copy text method
@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Apr 27, 2021
@kibanamachine
Copy link
Copy Markdown
Contributor

kibanamachine commented Apr 27, 2021

Looks like this PR has backport PRs but they still haven't been merged. Please merge them ASAP to keep the branches relatively in sync.

jloleysens added a commit that referenced this pull request Apr 28, 2021
@jloleysens
[Console] Fix copy as cURL (#97968) (#98161)
398949f 
* updated code to use Clipboard API rather than document.execCommand

* added link to deprecation of document.execCommand

* removed legacy copy text method
@kibanamachine kibanamachine removed the backport missing Added to PRs automatically when the are determined to be missing a backport. label Apr 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@yuliacech yuliacech yuliacech approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Feature:Console Dev Tools Console Feature Feature:Dev Tools release_note:fix Team:Kibana Management Dev Tools, Index Management, Upgrade Assistant, ILM, Ingest Node Pipelines, and more t// v7.12.2 v7.13.0 v7.14.0 v8.0.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Copy as cURL doesn't work on DevTools screen

4 participants

@jloleysens @elasticmachine @kibanamachine @yuliacech