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.

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

[Security Solution] Add CellActions (alpha version) component to ui_actions plugin #147434

Merged
merged 15 commits into from
Dec 20, 2022

Conversation

machadoum
Copy link
Member

@machadoum machadoum commented Dec 13, 2022

Summary

Create a CellActions component. It hooks into a UI-Actions trigger and displays all available actions.
It has two modes, Hover_Actions and Always_Visible.

You can run the storybook and take a look at the component: yarn storybook ui_actions or access https://ci-artifacts.kibana.dev/storybooks/pr-147434/226993c612bbe1719de6374219009bc69b0378d8/ui_actions/index.html

*** This component is still not in use.

Screenshot 2022-12-13 at 13 13 46

Screenshot 2022-12-13 at 13 13 30

Why?

The security Solution team is creating a generic UI component to allow teams to share actions between different plugins.
Initially, only the Security solution plugin will use this component and deprecate the Security solution custom implementation. Some actions that will be shared are: "copy to clipboard", "filter in", "filter out" and "add to timeline".

How to use it:

This package provides a uniform interface for displaying UI actions for a cell.
For the CellActions component to work, it must be wrapped by CellActionsContextProvider. Ideally, the wrapper should stay on the top of the rendering tree.

Example:

<CellActionsContextProvider
    // call uiActions.getTriggerCompatibleActions(triggerId, data)
    getCompatibleActions={getCompatibleActions}>
    ...
    <CellActions mode={CellActionsMode.HOVER_POPOVER} triggerId={MY_TRIGGER_ID} config={{ field: 'fieldName', value: 'fieldValue', fieldType: 'text' }}>
        Hover me
    </CellActions>
</CellActionsContextProvider>

CellActions component will display all compatible actions registered for the trigger id.

Checklist

@machadoum machadoum added Team:Threat Hunting Security Solution Threat Hunting Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:Threat Hunting:Explore v8.7.0 labels Dec 13, 2022
@machadoum machadoum self-assigned this Dec 13, 2022
@machadoum machadoum changed the title Add CellActions component to ui_actions plugin [Security Solution] Add CellActions (alpha version) component to ui_actions plugin Dec 13, 2022
@machadoum machadoum marked this pull request as ready for review December 13, 2022 15:20
@machadoum machadoum requested review from a team as code owners December 13, 2022 15:20
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-threat-hunting (Team:Threat Hunting)

@elasticmachine
Copy link
Contributor

Pinging @elastic/security-solution (Team: SecuritySolution)

@machadoum machadoum added the release_note:skip Skip the PR/issue when compiling release notes label Dec 13, 2022
@stephmilovic
Copy link
Contributor

@elasticmachine merge upstream

@stephmilovic
Copy link
Contributor

@elasticmachine merge upstream

}) => {
const actionProps = useMemo(
() => ({
iconType: action.getIconType(actionContext) as IconType,
Copy link
Contributor

Choose a reason for hiding this comment

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

would it make sense to update the Action type to make getIconType return IconType instead of string | undefined

Copy link
Member Author

Choose a reason for hiding this comment

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

Using IconTypeinstead of string makes sense to me. I think it is a string because EuiContextMenuItemIcon expects a string instead of IconType. So updating it might lead to cascading changes that could grow big.

Tagging @Dosant and @vadimkibana to collect their input.

Copy link
Contributor

Choose a reason for hiding this comment

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

Need to think about this. That would mean ui_actions plugin service layer would have a dependency on EUI, which maybe is fine, maybe not desirable.

Copy link
Contributor

@stephmilovic stephmilovic left a comment

Choose a reason for hiding this comment

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

Just a few nits. Tested in storybook and works as expected. Nicely done! LGTM

@machadoum machadoum force-pushed the siem-explore-145663-ui-action branch 2 times, most recently from e6c6a8d to 0d443a0 Compare December 14, 2022 10:39
export const CellActions: React.FC<CellActionsProps> = ({
field,
triggerId,
children,
Copy link
Contributor

Choose a reason for hiding this comment

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

What is the purpose for the children property? Trying to find the description in CellActionsProps.
Probably we just should name it different, something like triggeringElement or so.

Copy link
Member Author

Choose a reason for hiding this comment

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

The children property will be used for rendering the field content. CellActions wraps the cell content and provides extra functionality.

The children property is required for the hovering mode. CellActions wraps the children and displays a popover on hover.
Here is an example:

<CellActions mode={CellActionsMode.HOVER_POPOVER} triggerId={TRIGGER_ID} field={FIELD}>
    Hover me
</CellActions>

For inline mode, it isn't strictly required. But if it is provided, CellActions will display the action side-by-side with children.

We could consider exporting two components, one for hover and one for inline actions. But, in the first interaction, I decided to keep the API as simple as possible by exporting one component (We can easily change that).

Given that hover actions need to wrap the field value to work, children is the only property that allows the "wrapping" JSX syntax.

<WrappingComponent>
  <WrappedComponent />
</WrappingComponent>

It is also built into typescript. So if a component is typed with React.FC<>, it already has a property named children.

Copy link
Contributor

@YulNaumenko YulNaumenko left a comment

Choose a reason for hiding this comment

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

Overall the components design LGTM. I have some minor questions regarding the props naming, because we should clearly explain the purpose and capabilities of each.

@YulNaumenko
Copy link
Contributor

I think I can see the potential, but at the same time in our plugin implementing some basic functionality like copy to clipboard, filter in and filter out was super simple and very little code, that I'm afraid that using this might end up being the same amount of code or close... with the added complexity of understanding how to set this up, and that's only after someone would have made us aware of this existed in the first place...

@PhilippeOberti thank you for the detailed feedback. This is just an early phase of the design the new cell actions functionality, which we decided to split by the smaller PRs, without the touching or impacting existing code base till the final version will be ready. The idea is to have it simple, much simpler that it is now covered by timelines.getHoverActions interface.

Finally I feel that the PR should implement at least one usage of the functionality in many plugins, or the chances of it not being used is a lot higher... that would also be a good way for reviewers to test that it works as expected, and at the same time provide example(s) for less experience people (like me) and also show that the pattern is usable and useful.

We definitely will provide the documentation (and RFC) for the components when it will be ready for a wide usage and will do the cleanup of the existing HoverActions usage by replacing it with the CellActions component.

Copy link
Contributor

@semd semd left a comment

Choose a reason for hiding this comment

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

This component looks amazing. Thanks for taking the time to address the comments @machadoum.
I only left a couple of naming NITs, the component code overall LGTM!
Great job! 🚀

@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
uiActions 26 39 +13

Public APIs missing comments

Total count of every public API that lacks a comment. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats comments for more detailed information.

id before after diff
uiActions 92 97 +5

Public APIs missing exports

Total count of every type that is part of your API that should be exported but is not. This will cause broken links in the API documentation system. Target amount is 0. Run node scripts/build_api_docs --plugin [yourplugin] --stats exports for more detailed information.

id before after diff
uiActions 11 12 +1

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
uiActions 19.7KB 27.3KB +7.6KB
Unknown metric groups

API count

id before after diff
uiActions 135 140 +5

ESLint disabled in files

id before after diff
osquery 1 2 +1

ESLint disabled line counts

id before after diff
enterpriseSearch 19 21 +2
fleet 61 67 +6
osquery 109 115 +6
securitySolution 439 445 +6
uiActions 0 1 +1
total +21

Total ESLint disabled count

id before after diff
enterpriseSearch 20 22 +2
fleet 70 76 +6
osquery 110 117 +7
securitySolution 516 522 +6
uiActions 0 1 +1
total +22

History

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

cc @machadoum

@machadoum machadoum merged commit ed1f965 into elastic:main Dec 20, 2022
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Dec 20, 2022
crespocarlos pushed a commit to crespocarlos/kibana that referenced this pull request Dec 23, 2022
…ctions plugin (elastic#147434)

## Summary

Create a `CellActions` component. It hooks into a UI-Actions trigger and
displays all available actions.
It has two modes, Hover_Actions and Always_Visible. 

You can run the storybook and take a look at the component: `yarn
storybook ui_actions` or access
https://ci-artifacts.kibana.dev/storybooks/pr-147434/226993c612bbe1719de6374219009bc69b0378d8/ui_actions/index.html

*** This component is still not in use.

<img width="117" alt="Screenshot 2022-12-13 at 13 13 46"
src="https://user-images.githubusercontent.com/1490444/207316029-26c7bad8-ae39-48ba-8059-cbacf01a98aa.png">


<img width="224" alt="Screenshot 2022-12-13 at 13 13 30"
src="https://user-images.githubusercontent.com/1490444/207316024-0d7706c8-bd59-42e8-bf6d-b5648fc818fd.png">


#### Why?
The security Solution team is creating a generic UI component to allow
teams to share actions between different plugins.
Initially, only the Security solution plugin will use this component and
deprecate the Security solution custom implementation. Some actions that
will be shared are: "copy to clipboard", "filter in", "filter out" and
"add to timeline".



#### How to use it:
This package provides a uniform interface for displaying UI actions for
a cell.
For the `CellActions` component to work, it must be wrapped by
`CellActionsContextProvider`. Ideally, the wrapper should stay on the
top of the rendering tree.

Example:
```JSX
<CellActionsContextProvider
    // call uiActions.getTriggerCompatibleActions(triggerId, data)
    getCompatibleActions={getCompatibleActions}>
    ...
    <CellActions mode={CellActionsMode.HOVER_POPOVER} triggerId={MY_TRIGGER_ID} config={{ field: 'fieldName', value: 'fieldValue', fieldType: 'text' }}>
        Hover me
    </CellActions>
</CellActionsContextProvider>

```

`CellActions` component will display all compatible actions registered
for the trigger id.



### Checklist

- [x] Any text added follows [EUI's writing
guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses
sentence case text and includes [i18n
support](https://github.com/elastic/kibana/blob/main/packages/kbn-i18n/README.md)
- [x] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
- [ ] Any UI touched in this PR is usable by keyboard only (learn more
about [keyboard accessibility](https://webaim.org/techniques/keyboard/))
- [x] Any UI touched in this PR does not create any new axe failures
(run axe in browser:
[FF](https://addons.mozilla.org/en-US/firefox/addon/axe-devtools/),
[Chrome](https://chrome.google.com/webstore/detail/axe-web-accessibility-tes/lhdoppojpmngadmnindnejefpokejbdd?hl=en-US))
- [x] This renders correctly on smaller devices using a responsive
layout. (You can test this [in your
browser](https://www.browserstack.com/guide/responsive-testing-on-local-server))
- [x] This was checked for [cross-browser
compatibility](https://www.elastic.co/support/matrix#matrix_browsers)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting release_note:skip Skip the PR/issue when compiling release notes Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:Threat Hunting:Explore Team:Threat Hunting Security Solution Threat Hunting Team v8.7.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants