Skip to content

[Security Solution] expanded flyout#150240

Merged
PhilippeOberti merged 1 commit intomainfrom
expanded-flyout-part1-no-store
Feb 28, 2023
Merged

[Security Solution] expanded flyout#150240
PhilippeOberti merged 1 commit intomainfrom
expanded-flyout-part1-no-store

Conversation

@PhilippeOberti
Copy link
Copy Markdown
Contributor

@PhilippeOberti PhilippeOberti commented Feb 3, 2023 •
edited
Loading

Summary

This PR is the first of many implementing a new expanded flyout.

Mocks can be found here.

This first part was based on some preliminary work done by @michaelolo24 and can be seen in this draft PR.

Here's what's included in this PR:

  • add the securityFlyoutEnabled feature flag while we're working on the expanded flyout over multiple PRs
  • add a new Kibana package called kbn-expandable-flyout. The package contains the ExpandableFlyout UI component, a context and a set of methods to interact with the flyout. The packages takes care of managing the state (the flyout's layout)
  • skeleton of the expanded flyout with left, right and preview panels

TODO

  • add some missing code documentation
  • add unit tests
  • fix CI errors

How to test

  • add xpack.securitySolution.enableExperimental: ['securityFlyoutEnabled'] to the kibana.json file
  • go to the Alerts page, and click on the expand detail button on any row of the table
Screen.Recording.2023-02-01.at.5.10.45.PM.mov

https://github.com/elastic/security-team/issues/6064

Notes

  • this PR does NOT include any panels, so when testing it nothing would happen when clicking on the alert table row expand button (the recorded video above shows the behavior with tests data)

Thoughts

  • at the moment the width of each panel is set in pixels. We should probably think about converting it in percentage of the flyout width or something like that... Maybe some UIUX guidance is needed here.
  • also the width is currently a property of a panel level, but we might want to handle this at the flyout level instead
  • at the moment the code brings the flexibitly to put basically any panel anywhere (meaning we could in theory show the alert detail overview panel on the right, left or preview sections. I'm not sure that's necessary

What's next?

  • save/load to/from url
  • add the actual content (panels) of the flyout sections (like JSON view, table view...)
  • rework the width (pixels vs percentage)

@PhilippeOberti PhilippeOberti requested a review from a team as a code owner February 3, 2023 00:01
@PhilippeOberti PhilippeOberti added release_note:skip Skip the PR/issue when compiling release notes Team:Threat Hunting Security Solution Threat Hunting Team labels Feb 3, 2023
@elasticmachine
Copy link
Copy Markdown
Contributor

elasticmachine commented Feb 3, 2023

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

@PhilippeOberti PhilippeOberti changed the title [Security Solution] expanded flyout - No Redux store [Security Solution] expanded flyout Feb 6, 2023
@PhilippeOberti PhilippeOberti mentioned this pull request Feb 6, 2023
Closed
2 tasks
@PhilippeOberti PhilippeOberti force-pushed the expanded-flyout-part1-no-store branch 3 times, most recently from fa68b74 to f15b27c Compare February 7, 2023 04:24
michaelolo24
michaelolo24 reviewed Feb 7, 2023
View reviewed changes
michaelolo24
michaelolo24 reviewed Feb 7, 2023
View reviewed changes
lgestc
lgestc reviewed Feb 7, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@lgestc lgestc left a comment

Choose a reason for hiding this comment

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

LGTM, I've added some suggestions though

packages/kbn-expandable-flyout/src/components/translations.ts

import { i18n } from '@kbn/i18n';

export const BACK_BUTTON = i18n.translate('expandableFlyout.previewSection.backButton', {
Copy link
Copy Markdown
Contributor

@lgestc lgestc Feb 7, 2023

Choose a reason for hiding this comment

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

this is some cool approach right there. 😎

michaelolo24
michaelolo24 reviewed Feb 7, 2023
View reviewed changes
michaelolo24
michaelolo24 reviewed Feb 7, 2023
View reviewed changes
@PhilippeOberti PhilippeOberti force-pushed the expanded-flyout-part1-no-store branch 3 times, most recently from b36d1d7 to 65522b1 Compare February 8, 2023 23:04
@PhilippeOberti
Copy link
Copy Markdown
Contributor Author

PhilippeOberti commented Feb 8, 2023

@michaelolo24 I reverted the add test data commit to make sure the review is being done only on the important files, and also have the CI run on what matters. For testing purposes we can just reset the branch to right before the last commit and get the test data back, or cherry pick the second commit!

@PhilippeOberti PhilippeOberti force-pushed the expanded-flyout-part1-no-store branch 2 times, most recently from d67a1ac to b4f033f Compare February 9, 2023 22:26
@PhilippeOberti PhilippeOberti force-pushed the expanded-flyout-part1-no-store branch from b4f033f to f887a0f Compare February 23, 2023 14:56
@PhilippeOberti PhilippeOberti mentioned this pull request Feb 23, 2023
Merged
4 tasks
@PhilippeOberti PhilippeOberti added release_note:feature Makes this part of the condensed release notes Team:Threat Hunting:Investigations Security Solution Threat Hunting Investigations Team and removed release_note:skip Skip the PR/issue when compiling release notes labels Feb 23, 2023
@PhilippeOberti PhilippeOberti mentioned this pull request Feb 24, 2023
Merged
4 tasks
michaelolo24
michaelolo24 reviewed Feb 27, 2023
View reviewed changes
x-pack/plugins/security_solution/public/app/home/template_wrapper/index.tsx
</EuiThemeProvider>
</KibanaPageTemplate.BottomBar>
)}
<ExpandableFlyout registeredPanels={[]} onClose={() => {}} />
Copy link
Copy Markdown
Contributor

@michaelolo24 michaelolo24 Feb 27, 2023

Choose a reason for hiding this comment

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

👍🏾

michaelolo24
michaelolo24 approved these changes Feb 27, 2023
View reviewed changes
Copy link
Copy Markdown
Contributor

@michaelolo24 michaelolo24 left a comment

Choose a reason for hiding this comment

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

Sorry, it took so long to get back to this. The changes look great. Thanks for breaking it apart like this, LGTM! 💪🏾

@PhilippeOberti
[Security Solution ] security solution expanded flyout
324909f 
- feature flag in security_solution plugin
- new kbn-expandable-flyout package
- skeleton of the expanded flyout with left, right and preview panels
@PhilippeOberti PhilippeOberti force-pushed the expanded-flyout-part1-no-store branch from f887a0f to 324909f Compare February 27, 2023 23:40
@kibana-ci
Copy link
Copy Markdown

kibana-ci commented Feb 28, 2023

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] FTR Configs #38 / console app console app with comments with single line comments should allow in request body, using //

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
securitySolution 3713 3723 +10

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
@kbn/expandable-flyout - 4 +4

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
securitySolution 15.7MB 15.7MB +21.0KB

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
@kbn/expandable-flyout - 3 +3

Page load bundle

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

id before after diff
securitySolution 53.2KB 53.2KB +25.0B
Unknown metric groups

API count

id before after diff
@kbn/expandable-flyout - 13 +13

ESLint disabled line counts

id before after diff
securitySolution 428 430 +2

Total ESLint disabled count

id before after diff
securitySolution 506 508 +2

History

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

@PhilippeOberti PhilippeOberti merged commit 4aa0961 into main Feb 28, 2023
@PhilippeOberti PhilippeOberti deleted the expanded-flyout-part1-no-store branch February 28, 2023 03:02
@kibanamachine kibanamachine added the backport:skip This PR does not require backporting label Feb 28, 2023
bmorelli25 pushed a commit to bmorelli25/kibana that referenced this pull request Mar 10, 2023
@PhilippeOberti @bmorelli25
[Security Solution] expanded flyout (elastic#150240)
29d193f
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lgestc lgestc lgestc left review comments

@kqualters-elastic kqualters-elastic kqualters-elastic left review comments

@michaelolo24 michaelolo24 michaelolo24 approved these changes

Assignees

No one assigned

Labels

backport:skip This PR does not require backporting release_note:feature Makes this part of the condensed release notes Team:Threat Hunting:Investigations Security Solution Threat Hunting Investigations Team Team:Threat Hunting Security Solution Threat Hunting Team v8.8.0

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

@PhilippeOberti @elasticmachine @kibana-ci @lgestc @michaelolo24 @kqualters-elastic @kibanamachine