Skip to content

update copy styling#79313

Merged
michaelolo24 merged 6 commits intoelastic:masterfrom
michaelolo24:add-copy-functionality
Oct 5, 2020
Merged

update copy styling#79313
michaelolo24 merged 6 commits intoelastic:masterfrom
michaelolo24:add-copy-functionality

Conversation

@michaelolo24
Copy link
Copy Markdown
Contributor

@michaelolo24 michaelolo24 commented Oct 2, 2020 •
edited
Loading

Summary

Adds the ability to copy fields from the resolver panel

  • Node List:

panel-copypaste

  • Node Detail:

copy_node-details

  • Event Detail:

copy_event-details

For maintainers

@michaelolo24 michaelolo24 marked this pull request as ready for review October 2, 2020 16:24
@michaelolo24 michaelolo24 requested review from a team as code owners October 2, 2020 16:24
@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Oct 2, 2020

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

@michaelolo24
Copy link
Copy Markdown
Contributor Author

michaelolo24 commented Oct 5, 2020

@elasticmachine merge upstream

oatkiller
oatkiller suggested changes Oct 5, 2020
View reviewed changes
Copy link
Copy Markdown
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.

let's chat

x-pack/plugins/security_solution/public/resolver/view/panels/panel_content_utilities.tsx Outdated
Copy link
Copy Markdown
Contributor

@oatkiller oatkiller Oct 5, 2020

Choose a reason for hiding this comment

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

i18n.translate just returns a string. You shouldn't need to store a reference to it. Can you just place the expression inline in the react component instead?

Copy link
Copy Markdown
Contributor Author

@michaelolo24 michaelolo24 Oct 5, 2020

Choose a reason for hiding this comment

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

Yep

x-pack/plugins/security_solution/public/resolver/view/panels/panel_content_utilities.tsx Outdated
Copy link
Copy Markdown
Contributor

@oatkiller oatkiller Oct 5, 2020

Choose a reason for hiding this comment

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

Can you please move this to its own module.

Copy link
Copy Markdown
Contributor Author

@michaelolo24 michaelolo24 Oct 5, 2020

Choose a reason for hiding this comment

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

will do

x-pack/plugins/security_solution/public/resolver/view/panels/panel_content_utilities.tsx Outdated
Copy link
Copy Markdown
Contributor

@oatkiller oatkiller Oct 5, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Let's discuss how to handle this.

Copy link
Copy Markdown
Contributor Author

@michaelolo24 michaelolo24 Oct 5, 2020

Choose a reason for hiding this comment

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

no, it utilizes an existing component within security_solution. I can spin up our own minimal version though?

x-pack/plugins/security_solution/public/resolver/view/panels/panel_content_utilities.tsx Outdated
Copy link
Copy Markdown
Contributor

@oatkiller oatkiller Oct 5, 2020

Choose a reason for hiding this comment

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

Can this be opened with the keyboard?

Copy link
Copy Markdown
Contributor Author

@michaelolo24 michaelolo24 Oct 5, 2020

Choose a reason for hiding this comment

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

Need to test it. But we may be changing the way it works...talking to ui/ux at the moment

bkimmel
bkimmel reviewed Oct 5, 2020
View reviewed changes
x-pack/plugins/security_solution/public/resolver/view/panels/panel_content_utilities.tsx Outdated
Copy link
Copy Markdown
Contributor

@bkimmel bkimmel Oct 5, 2020

Choose a reason for hiding this comment

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

🔴 for the following reason:

https://www.w3.org/WAI/WCAG21/Understanding/keyboard.html

"All functionality of the content is operable through a keyboard interface..."

Following that guidance, "Copy" links can't be tied/triggered to "hover"/"mouseEnter"/"mouseLeave" states or other states that can't be accessed with a keyboard. We could probably wire up something comparable with "focus"/"lose focus" so it can be done with keyboards although it's not as straightforward as you might think. IIRC there might be some things in EUI that help with this.

Copy link
Copy Markdown
Contributor Author

@michaelolo24 michaelolo24 Oct 5, 2020

Choose a reason for hiding this comment

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

👍 thanks! fyi @monina-n @lindseypoli

Copy link
Copy Markdown
Contributor Author

@michaelolo24 michaelolo24 Oct 5, 2020

Choose a reason for hiding this comment

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

We decided that while it is not accessible at the moment, there are plans to make timeline more accessible. Given that, in addition to not wanting to give the users a new behavior to learn or clutter the ui, we're gonna match how timeline does it for now

bkimmel
bkimmel reviewed Oct 5, 2020
View reviewed changes
x-pack/plugins/security_solution/public/resolver/view/panels/panel_content_utilities.tsx Outdated
Copy link
Copy Markdown
Contributor

@bkimmel bkimmel Oct 5, 2020

Choose a reason for hiding this comment

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

❔ doc comment on export

bkimmel
bkimmel reviewed Oct 5, 2020
View reviewed changes
x-pack/plugins/security_solution/public/resolver/view/panels/panel_content_utilities.tsx Outdated
Copy link
Copy Markdown
Contributor

@bkimmel bkimmel Oct 5, 2020

Choose a reason for hiding this comment

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

❔ does this need a memo?

Copy link
Copy Markdown
Contributor Author

@michaelolo24 michaelolo24 Oct 5, 2020

Choose a reason for hiding this comment

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

added, thanks

@michaelolo24 michaelolo24 force-pushed the add-copy-functionality branch from 58e4718 to 264f9f8 Compare October 5, 2020 18:19
@michaelolo24
Copy link
Copy Markdown
Contributor Author

michaelolo24 commented Oct 5, 2020

@oatkiller - added some tests, but it will probably change based on how we end up deciding to implement this copy functionality, if we implement it at all here. Also, I was gonna test the copy directly, but ran into an issue with a dependency of copy-to-clipboard, so just mocked the module itself. Was considering just implementing our own copy functionality using navigator.clipboard.writeText and then testing that directly, but ie11 doesn't support it

oatkiller
oatkiller approved these changes Oct 5, 2020
View reviewed changes
Copy link
Copy Markdown
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.

Let's discuss the ResolverPanelContext later this week. I'm not sure I understand the motivation for the design or the implementation.

@kibanamachine
Copy link
Copy Markdown
Contributor

kibanamachine commented Oct 5, 2020

💚 Build Succeeded

Metrics [docs]

@kbn/optimizer bundle module count

id before after diff
securitySolution 1998 2000 +2

async chunks size

id before after diff
securitySolution 10.3MB 10.3MB +12.4KB

History

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

@michaelolo24 michaelolo24 merged commit 4d4e53f into elastic:master Oct 5, 2020
@michaelolo24 michaelolo24 deleted the add-copy-functionality branch October 5, 2020 23:13
@michaelolo24 michaelolo24 mentioned this pull request Oct 5, 2020
Merged
michaelolo24 added a commit to michaelolo24/kibana that referenced this pull request Oct 5, 2020
@michaelolo24
update copy styling (elastic#79313)
dec9f0b
michaelolo24 added a commit that referenced this pull request Oct 6, 2020
@michaelolo24
[7.x] update copy styling (#79313) (#79593)
a14c2ae
gmmorris added a commit to gmmorris/kibana that referenced this pull request Oct 6, 2020
@gmmorris
Merge branch 'master' into alerts/correct-name-of-built-inalerts-feature
791ffca 
* master: (85 commits)
  Refactor attribute service (elastic#78414)
  [APM] Add default message to alerts. (elastic#78930)
  [Discover] Modify columns and sort when switching index pattern (elastic#74336)
  Document ts project references setup (elastic#78586)
  build all ts refs in single kbn:bootstrap (elastic#79438)
  [TSVB] Allow string fields on value count aggregation (elastic#79267)
  [SECURITY SOLUTION] Investigate EQL signal in timeline (elastic#79049)
  [Fleet] Add loading spinner to page headers (elastic#79568)
  [Security Solution][Resolver] Resolver query panel load more (elastic#79160)
  Add type row to monitor detail page. (elastic#79556)
  Remove license refresh from setup (elastic#79518)
  [docker] add reporting fonts (elastic#74806)
  [SECURITY_SOLUTION][ENDPOINT] Add info about trusted apps to the page subtitle + create flyout (elastic#79558)
  Trim Hash value before validating it (elastic#79545)
  Warn users when security is not configured (elastic#78545)
  update copy styling (elastic#79313)
  Update dependency @elastic/charts to v23.1.1 (elastic#78459)
  Introduce geo-threshold alerts (elastic#76285)
  elastic#76920 Show base breadcrumb when there is an error booting the app (elastic#79571)
  remove react-intl from kibana and keep it inside only i18n package (elastic#78956)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

3 more reviewers

@bkimmel bkimmel bkimmel left review comments

@oatkiller oatkiller oatkiller approved these changes

@monina-n monina-n monina-n approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Feature:Resolver Security Solution Resolver feature release_note:enhancement v7.10.0 v8.0.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@michaelolo24 @elasticmachine @kibanamachine @oatkiller @bkimmel @monina-n