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][Expandable flyout] - add onClose to the API to let developers know when the expandable flyout is being closed #183553

Merged

Conversation

PhilippeOberti
Copy link
Contributor

@PhilippeOberti PhilippeOberti commented May 15, 2024

Summary

This PR propagates the onClose callback from EuiFlyout outside of the package, to allow application's code to respond to the flyout being closed.

Will help
https://github.com/elastic/security-team/issues/7670 and #179520

Checklist

  • Documentation was added for features that require explanation or tutorials

Previous implementation

This PR adds an onClose property to the kbn-expandable-flyout package. Until now, there was not any way to know when the expandable flyout was being closed. This was due to the fact that the expandable flyout is a wrapper of the EUI flyout, and has its own api.

Approach

This approach implements an rxjs observable that the kbn-expandable-flyout package provides through its API. The observable is triggered every time the flyout closes. Components can listen to the observable and run the code they need.
Because there can be multiple flyouts open at the same time, the observable emits the id of the flyout. It its current state within Security Solution, only 3 options here can be emitted:

  • flyout for the expandable flyouts open on the alerts page, the cases page or the explore pages (host/user...)
  • timelineFlyout for the expandable flyout from a Timeline
  • memory for the expandable flyouts opened in memory mode (like the one on the rule creation page)

Notes:

One downside of this approach is we're running the risk to have developer forgetting to unsubscribe from the observable and therefore introducing memory leaks...
An alternative approach PR will be open shortly to investigate using Events instead of Observables.

@elasticmachine
Copy link
Contributor

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

@PhilippeOberti
Copy link
Contributor Author

@christineweng @logeekal @lgestc I opened this new PR in favor of this other one so we can focus only on the onClose API exposed by the kbn-expandable-flyout.

It will be easier to discuss the rxjs Observable vs Event approach

…t developers know when the expandable flyout is being closed
…PI to let developers know when the expandable flyout is being closed"

This reverts commit 15d2fdc.
@PhilippeOberti PhilippeOberti requested a review from a team as a code owner June 4, 2024 18:24
@PhilippeOberti
Copy link
Contributor Author

@christineweng @logeekal @lgestc I opened this new PR in favor of this other one so we can focus only on the onClose API exposed by the kbn-expandable-flyout.

It will be easier to discuss the rxjs Observable vs Event approach

Ok after talking to @lgestc more, he convinced me to implement the simplest way possible. The package won't contain any custom code, we just propagate the onClose callback out and let the application's code handle it the way it wants

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.

LGTM

@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

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
securitySolution 13.6MB 13.6MB +31.0B
Unknown metric groups

API count

id before after diff
@kbn/expandable-flyout 38 39 +1

History

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

@PhilippeOberti PhilippeOberti merged commit cc9a5b1 into elastic:main Jun 6, 2024
18 checks passed
@kibanamachine kibanamachine added the backport:skip This commit does not require backporting label Jun 6, 2024
@PhilippeOberti PhilippeOberti deleted the expandable-flyout-on-close branch June 6, 2024 17:55
@PhilippeOberti PhilippeOberti added release_note:skip Skip the PR/issue when compiling release notes and removed release_note:enhancement labels Aug 2, 2024
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:Threat Hunting:Investigations Security Solution Investigations Team v8.15.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants