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

[Security Solution][Resolver] Replace copy-to-clipboard with native navigator.clipboard #80193

Merged
merged 4 commits into from
Oct 13, 2020
Merged

[Security Solution][Resolver] Replace copy-to-clipboard with native navigator.clipboard #80193

merged 4 commits into from
Oct 13, 2020

Conversation

michaelolo24
Copy link
Contributor

@michaelolo24 michaelolo24 commented Oct 12, 2020 •
edited
Loading

Summary

This replaces the clipboard package with the native navigator.clipboard api. For reference on writeText() https://developer.mozilla.org/en-US/docs/Web/API/Clipboard/writeText and browser availability: https://caniuse.com/?search=writetext

@michaelolo24 michaelolo24 added Feature:Resolver Security Solution Resolver feature Team:Threat Hunting Security Solution Threat Hunting Team v7.11.0 v8.0.0 release_note:skip Skip the PR/issue when compiling release notes labels Oct 12, 2020
@michaelolo24 michaelolo24 changed the title [use native copy functionality [Security Solution][Resolver] Replace copy-to-clipboard with native navigator.clipboard Oct 12, 2020
@michaelolo24 michaelolo24 marked this pull request as ready for review October 12, 2020 17:26
@michaelolo24 michaelolo24 requested review from a team as code owners October 12, 2020 17:26
@elasticmachine
Copy link
Contributor

Pinging @elastic/endpoint-app-team (Feature:Resolver)

@michaelolo24 michaelolo24 marked this pull request as draft October 12, 2020 17:26
@michaelolo24 michaelolo24 marked this pull request as ready for review October 12, 2020 17:36
@michaelolo24
Copy link
Contributor Author

@elasticmachine merge upstream

bkimmel
bkimmel reviewed Oct 12, 2020
View reviewed changes
x-pack/plugins/security_solution/public/resolver/view/panels/copyable_panel_field.tsx Outdated
titleSummary={textToCopy}
<EuiToolTip content={TRANSLATED_COPY_TO_CLIPBOARD}>
<EuiButtonIcon
aria-label={TRANSLATED_COPY_TO_CLIPBOARD}
Copy link
Contributor

Choose a reason for hiding this comment

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

❔ I think you could either put the text that's going to be copied in that aria-label like Copy ${textToCopy} to clipboard or you could maybe add an aria-description="${texttoCopy}" .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

aria-description doesn't seem to be valid. I can change it to Copy ${textToCopy} to clipboard?

oatkiller
oatkiller reviewed Oct 12, 2020
View reviewed changes
x-pack/plugins/security_solution/public/resolver/view/panels/copyable_panel_field.tsx Outdated
@@ -34,6 +34,12 @@ const StyledCopyableField = styled.div<StyledCopyableField>`
}
`;

const TRANSLATED_COPY_TO_CLIPBOARD = i18n.translate(
Copy link
Contributor

Choose a reason for hiding this comment

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

can you just put this inline since its a string, or otherwise use a camel case variable name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yep

oatkiller
oatkiller reviewed Oct 12, 2020
View reviewed changes
x-pack/plugins/security_solution/public/resolver/view/panels/copyable_panel_field.tsx Outdated
<EuiButtonIcon
aria-label={TRANSLATED_COPY_TO_CLIPBOARD}
color="text"
data-test-subj="clipboard"
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this data-test-subj correct? In the old code it was namespaced under resolver:...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, yea, I fixed it.

oatkiller
oatkiller reviewed Oct 12, 2020
View reviewed changes
x-pack/plugins/security_solution/public/resolver/view/panels/copyable_panel_field.tsx Outdated
@@ -59,6 +66,22 @@ export const CopyablePanelField = memo(

const onMouseLeave = () => setIsOpen(false);

const onClick = async (event: React.MouseEvent<HTMLButtonElement>) => {
event.preventDefault();
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the purpose in preventing default and stopping propagation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

copypasta. stopPropagation would be useful if we also had the drag and drop functionality, but alas we aren't there yet

oatkiller
oatkiller reviewed Oct 12, 2020
View reviewed changes
Copy link
Contributor

@oatkiller oatkiller left a comment

Choose a reason for hiding this comment

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

Thanks for the change. I added a few comments. Let me know if they don't make sense.

x-pack/plugins/security_solution/public/resolver/view/panel.test.tsx Outdated
@@ -36,6 +31,14 @@ describe(`Resolver: when analyzing a tree with no ancestors and two children and

beforeEach(() => {
// create a mock data access layer
Object.assign(navigator, {
clipboard: {
writeText: () => {},
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of doing this, would you add writeText to the side effect simulator?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

oatkiller
oatkiller approved these changes Oct 13, 2020
View reviewed changes
Copy link
Contributor

@oatkiller oatkiller left a comment

Choose a reason for hiding this comment

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

Thank you so much for this

@kibanamachine
Copy link
Contributor

💛 Build succeeded, but was flaky

Test Failures

Creates a timeline.Timelines Creates a timeline

Link to Jenkins

Stack Trace

Failed Tests Reporter:
  - Test has failed 9 times on tracked branches: https://github.com/elastic/kibana/issues/79389

CypressError: `cy.wait()` could not find a registered alias for: `@timeline`.
You have not aliased anything yet.
    at aliasNotFoundFor (http://elastic:changeme@localhost:6151/__cypress/runner/cypress_runner.js:150289:22)
    at $Cy.getAlias (http://elastic:changeme@localhost:6151/__cypress/runner/cypress_runner.js:150252:7)
    at waitForXhr (http://elastic:changeme@localhost:6151/__cypress/runner/cypress_runner.js:159965:25)
    at http://elastic:changeme@localhost:6151/__cypress/runner/cypress_runner.js:160054:14
    at tryCatcher (http://elastic:changeme@localhost:6151/__cypress/runner/cypress_runner.js:10325:23)
    at MappingPromiseArray._promiseFulfilled (http://elastic:changeme@localhost:6151/__cypress/runner/cypress_runner.js:7445:38)
    at MappingPromiseArray.PromiseArray._iterate (http://elastic:changeme@localhost:6151/__cypress/runner/cypress_runner.js:8647:31)
    at MappingPromiseArray.init (http://elastic:changeme@localhost:6151/__cypress/runner/cypress_runner.js:8611:10)
    at MappingPromiseArray._asyncInit (http://elastic:changeme@localhost:6151/__cypress/runner/cypress_runner.js:7414:10)
    at _drainQueueStep (http://elastic:changeme@localhost:6151/__cypress/runner/cypress_runner.js:5036:12)
    at _drainQueue (http://elastic:changeme@localhost:6151/__cypress/runner/cypress_runner.js:5025:9)
    at Async.../../node_modules/bluebird/js/release/async.js.Async._drainQueues (http://elastic:changeme@localhost:6151/__cypress/runner/cypress_runner.js:5041:5)
    at Async.drainQueues (http://elastic:changeme@localhost:6151/__cypress/runner/cypress_runner.js:4911:14)
From Your Spec Code:
    at Context.eval (http://localhost:6151/__cypress/tests?p=cypress/integration/timeline_creation.spec.ts:13497:35)

Metrics [docs]

async chunks size

id before after diff
securitySolution 10.6MB 10.6MB +3.2KB

History

  • 💚 Build #81159 succeeded 6d517ab20b3ab39c6bc17ec9d97a1e7f48992c1f
  • 💔 Build #81151 failed fadbe51525628114ca05ac2b6383e27313c43b7f
  • 💔 Build #81143 failed 8fdeef2ed4607554afe5d818f85a0282557cde10

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

@michaelolo24 michaelolo24 merged commit 5bf7966 into elastic:master Oct 13, 2020
@michaelolo24 michaelolo24 deleted the use-native-copy branch October 13, 2020 15:41
@michaelolo24 michaelolo24 mentioned this pull request Oct 13, 2020
Merged
michaelolo24 added a commit to michaelolo24/kibana that referenced this pull request Oct 13, 2020
@michaelolo24
[Security Solution][Resolver] Replace copy-to-clipboard with native n…
e1442e2 
…avigator.clipboard (elastic#80193)
jloleysens added a commit to jloleysens/kibana that referenced this pull request Oct 13, 2020
@jloleysens
Merge branch 'master' of github.com:elastic/kibana into ilm/migrate-h…
200b6d4 
…otphase-to-formlib

* 'master' of github.com:elastic/kibana: (59 commits)
  [Security Solution][Resolver] Replace copy-to-clipboard with native navigator.clipboard (elastic#80193)
  [Security Solution] Reduce initial bundle size (elastic#78992)
  [Security Solution][Resolver] Fix Resize node box-shadow bug (elastic#80223)
  Move observability content (elastic#79978)
  skip flaky suite (elastic#79389)
  removing kibana_datatable` in favor of `datatable` (elastic#75184)
  [ML] Fixes for anomaly swim lane  (elastic#80299)
  [Lens] Smokescreen lens test unskip (elastic#80190)
  Improved AlertsClient tests structure by splitting a huge alerts_client.tests.ts file into a specific files defined by its responsibility. (elastic#80088)
  [APM] React key warning when opening popover with external resources (elastic#80328)
  [Step 1] use Observables on server search API (elastic#79874)
  Apply back pressure in Task Manager whenever Elasticsearch responds with a 429 (elastic#75666)
  [Lens] Leverage original http request error (elastic#79831)
  [Security Solution][Case] Improve ServiceConnectorCaseParams type (elastic#80109)
  [SECURITY_SOLUTION] Fix query on alert histogram (elastic#80219)
  [DOCS] Update ingest node pipelines doc (elastic#79187)
  [Ingest Manager] Split up OpenAPI spec file  (elastic#80107)
  [SECURITY_SOLUTION][ENDPOINT] Fix label on Trusted App create name field (elastic#80001)
  [Ingest Manager] Fix agent policy bump revision to create only one POLICY_CHANGE action (elastic#80081)
  Grid layout fixes (elastic#80305)
  ...

# Conflicts:
#	x-pack/plugins/index_lifecycle_management/public/application/sections/edit_policy/components/phases/shared/data_tier_allocation_field.tsx
#	x-pack/plugins/index_lifecycle_management/public/shared_imports.ts
michaelolo24 added a commit that referenced this pull request Oct 13, 2020
@michaelolo24
[7.x] [Security Solution][Resolver] Replace copy-to-clipboard with na…
b81bce9 
…tive navigator.clipboard (#80193) (#80369)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bkimmel bkimmel bkimmel approved these changes

@oatkiller oatkiller oatkiller approved these changes

Assignees
No one assigned
Labels
Feature:Resolver Security Solution Resolver feature release_note:skip Skip the PR/issue when compiling release notes Team:Threat Hunting Security Solution Threat Hunting Team v7.11.0 v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@michaelolo24 @elasticmachine @kibanamachine @oatkiller @bkimmel