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.

Sign up for GitHub

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

Jump to bottom

[Bug] fix copy as curl #1472

Merged
merged 2 commits into from
Apr 21, 2022
Merged

[Bug] fix copy as curl #1472

merged 2 commits into from
Apr 21, 2022

Conversation

kavilla
Copy link
Member

@kavilla kavilla commented Apr 16, 2022 •
edited
Loading

Description

Copy as curl doesn't work in Dev Tools console due to usage of a
deprecated method:

https://developer.mozilla.org/en-US/docs/Web/API/Document/queryCommandSupported#browser_compatibility

Signed-off-by: Kawika Avilla kavilla414@gmail.com

Issues Resolved

#1471

Check List

  • New functionality includes testing.
    • All tests pass
      • yarn test:jest
      • yarn test:jest_integration
      • yarn test:ftr
  • New functionality has been documented.
  • Commits are signed per the DCO using --signoff

@kavilla
[Bug] fix copy as curl
e66bb05 
Copy as curl doesn't work in Dev Tools console due to usage of a
deprecated method:

https://developer.mozilla.org/en-US/docs/Web/API/Document/queryCommandSupported#browser_compatibility

Issue Resolved:
opensearch-project#1471

Signed-off-by: Kawika Avilla <kavilla414@gmail.com>
@kavilla kavilla requested a review from a team as a code owner April 16, 2022 10:12
@kavilla kavilla added bug Something isn't working v2.0.0 labels Apr 16, 2022
@kavilla kavilla linked an issue Apr 16, 2022 that may be closed by this pull request
[BUG] copy as curl doesn't work in Dev Tools #1471
Closed
tmarkley
tmarkley reviewed Apr 18, 2022
View reviewed changes
Copy link
Contributor

@tmarkley tmarkley left a comment

Choose a reason for hiding this comment

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

Was this found during smoke testing 2.0? Can we write tests for this?

mihirsoni
mihirsoni reviewed Apr 18, 2022
View reviewed changes
src/plugins/console/public/application/components/console_menu.tsx Outdated
Comment on lines 79 to 82
async copyText(text: string) {
if (window.navigator?.clipboard) {
await window.navigator.clipboard.writeText(text);
}
Copy link
Contributor

@mihirsoni mihirsoni Apr 18, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Will removing old logic drop support in old version, would be great to add a comment.

Copy link
Collaborator

Choose a reason for hiding this comment

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

According to caniuse, clipboard is supported by the browsers updated at least in the passed 3 years. Barring IE, all those that supported execCommand do support clipboard.

@boktorbb
Copy link
Contributor

boktorbb commented Apr 21, 2022 •
edited
Loading

Was this found during smoke testing 2.0? Can we write tests for this?

Writing a test for this doesn't seem to be feasible since the tests need read and write permissions that they do not have to be able to access clipboard values in headless mode, which is the primary case that the tests run in. The DOM doesn't have the values required. This is shown by this error: Uncaught DOMException: Write permission denied. I don't think a unit test would be good here either because mocking the clipboard value doesn't actually tell us that the functionality worked correctly and will work every time regardless of the real behavior. I think we should move forward with this PR and possibly take a backlog item to dive deeper into this if we deem it as a priority.

boktorbb
boktorbb previously approved these changes Apr 21, 2022
View reviewed changes
Copy link
Contributor

@boktorbb boktorbb left a comment

Choose a reason for hiding this comment

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

Looks good. I think taking a backlog item for looking into a good way of testing this type of functionality is the right move

Adds browser compatibility comment
705647d 
Signed-off-by: Bishoy Boktor <boktorbb@amazon.com>
@kavilla kavilla merged commit 376ba8c into opensearch-project:main Apr 21, 2022
opensearch-trigger-bot bot pushed a commit that referenced this pull request Apr 21, 2022
@kavilla @github-actions
[Bug] fix copy as curl (#1472)
dd243f3 
* [Bug] fix copy as curl

Copy as curl doesn't work in Dev Tools console due to usage of a
deprecated method:

https://developer.mozilla.org/en-US/docs/Web/API/Document/queryCommandSupported#browser_compatibility

Issue Resolved:
#1471

Signed-off-by: Kawika Avilla <kavilla414@gmail.com>

* Adds browser compatibility comment

Signed-off-by: Bishoy Boktor <boktorbb@amazon.com>

Co-authored-by: Bishoy Boktor <boktorbb@amazon.com>
(cherry picked from commit 376ba8c)
@opensearch-trigger-bot opensearch-trigger-bot bot mentioned this pull request Apr 21, 2022
Merged
@kavilla kavilla deleted the avillk/copy_as_curl branch April 21, 2022 01:58
kavilla added a commit that referenced this pull request Apr 21, 2022
@opensearch-trigger-bot @kavilla
[Bug] fix copy as curl (#1472) (#1483)
39d1641 
* [Bug] fix copy as curl

Copy as curl doesn't work in Dev Tools console due to usage of a
deprecated method:

https://developer.mozilla.org/en-US/docs/Web/API/Document/queryCommandSupported#browser_compatibility

Issue Resolved:
#1471

Signed-off-by: Kawika Avilla <kavilla414@gmail.com>

* Adds browser compatibility comment

Signed-off-by: Bishoy Boktor <boktorbb@amazon.com>

Co-authored-by: Bishoy Boktor <boktorbb@amazon.com>
(cherry picked from commit 376ba8c)

Co-authored-by: Kawika Avilla <kavilla414@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tmarkley tmarkley tmarkley left review comments

@mihirsoni mihirsoni mihirsoni left review comments

@boktorbb boktorbb boktorbb approved these changes

@AMoo-Miki AMoo-Miki AMoo-Miki approved these changes

Assignees

@boktorbb boktorbb

Labels
backport 2.x bug Something isn't working v2.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[BUG] copy as curl doesn't work in Dev Tools
5 participants
@kavilla @boktorbb @mihirsoni @AMoo-Miki @tmarkley