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][Notes] - add note block to alert defaitls flyout header #193373

Conversation

PhilippeOberti
Copy link
Contributor

@PhilippeOberti PhilippeOberti commented Sep 18, 2024

Summary

This PR adds a new section to the alert details flyout header to show how many notes have been added to the event or alert.

Note: This UI change only applies to the alert flyout. For event flyout, as we currently don't have this block section, we need to wait for UIUX to come up with a solution to show this note count

Here are the 2 states of this new section:

Loading Showing data
Screenshot 2024-09-18 at 6 13 04 PM Screenshot 2024-09-18 at 6 13 25 PM

The UI updates automatically whenever a new note is added.

Screen.Recording.2024-09-18.at.6.28.04.PM.mov

Also the flyout renders the values on 1 or 2 rows, depending of the width of the flyout itself

Screen.Recording.2024-09-18.at.6.27.08.PM.mov

The PR also introduce a small UI change in the way we're showing the Status, Risk score, Assignees and the new Notes section. Instead of being a single block with vertical divider between the values, we're now show individual blocks.

#193342

TODO

  • use the number format component on the notes number

Checklist

@PhilippeOberti PhilippeOberti requested a review from a team as a code owner September 18, 2024 23:34
@elasticmachine
Copy link
Contributor

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

@PhilippeOberti PhilippeOberti added the release_note:skip Skip the PR/issue when compiling release notes label Sep 18, 2024
@PhilippeOberti PhilippeOberti force-pushed the notes-add-block-to-expandable-flyout branch from 9bd981d to 3a747e0 Compare September 19, 2024 04:30
@PhilippeOberti PhilippeOberti added the backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) label Sep 19, 2024
@PhilippeOberti PhilippeOberti force-pushed the notes-add-block-to-expandable-flyout branch from 3a747e0 to 9ace735 Compare September 19, 2024 14:00
@PhilippeOberti PhilippeOberti marked this pull request as draft September 19, 2024 14:38
@PhilippeOberti PhilippeOberti marked this pull request as ready for review September 19, 2024 15:23
@PhilippeOberti PhilippeOberti changed the title [Security Solution][Notes] - add note block to alert and event detail flyout header [Security Solution][Notes] - add note block to alert defaitls flyout header Sep 19, 2024
@PhilippeOberti PhilippeOberti force-pushed the notes-add-block-to-expandable-flyout branch 2 times, most recently from d9ef96d to abec400 Compare September 19, 2024 16:23
Copy link
Contributor

@christineweng christineweng left a comment

Choose a reason for hiding this comment

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

Code and desk testing LGTM! Comparing the current UI, the new design is taller (wider gutter maybe?). What do you think about making it more compact?

image

@PhilippeOberti
Copy link
Contributor Author

PhilippeOberti commented Sep 19, 2024

Code and desk testing LGTM! Comparing the current UI, the new design is taller (wider gutter maybe?). What do you think about making it more compact?

Yeah I noticed this also. I tried a smaller padding size for the EuiPanel but didn't like it.
Here's what I have now (paddingSize="m")
Screenshot 2024-09-19 at 2 26 49 PM

And here is with paddingSize="s"
Screenshot 2024-09-19 at 2 26 24 PM

I had also tried with a custom padding like we were doing in the previous one block way (padding: ${euiTheme.size.m} ${euiTheme.size.s};)
Screenshot 2024-09-19 at 2 30 19 PM

Do you have one that you prefer? At first I didn't like the one with the smallest padding (the middle one above), but it's growing on me now!

@PhilippeOberti
Copy link
Contributor Author

Ok the decision was to go with the smaller paddingSize="s" for each block, which allowed us to bring the minWidthor each block from 300px down to 280px. This is a gain of 80px for the flyout's width before switching from 1 row 4 blocks to 2 rows 2 blocks each!

@PhilippeOberti PhilippeOberti force-pushed the notes-add-block-to-expandable-flyout branch from 899da73 to a063f0d Compare September 19, 2024 21:37
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
securitySolution 5811 5813 +2

Async chunks

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

id before after diff
securitySolution 20.4MB 20.4MB +3.5KB

History

  • 💔 Build #235756 failed 899da737478d9091a08b7a34e8df84f9a7d0db31
  • 💛 Build #235707 was flaky abec400619f5216cdea0cdec959c753401c516a8
  • 💔 Build #235701 failed d9ef96d033ce29dfa1b9be5c26f391f56fc31dab
  • 💔 Build #235622 failed 9ace735ec0bd8a275160595ede1454a98c27e902
  • 💔 Build #235442 failed 3a747e05109b7e59ffbdc71a9574ba227107e7f6

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

@PhilippeOberti PhilippeOberti merged commit 258adf5 into elastic:main Sep 20, 2024
40 checks passed
@PhilippeOberti PhilippeOberti deleted the notes-add-block-to-expandable-flyout branch September 20, 2024 00:39
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Sep 20, 2024
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
8.x

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

kibanamachine added a commit that referenced this pull request Sep 20, 2024
…lyout header (#193373) (#193502)

# Backport

This will backport the following commits from `main` to `8.x`:
- [[Security Solution][Notes] - add note block to alert defaitls flyout
header (#193373)](#193373)

<!--- Backport version: 9.4.3 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT [{"author":{"name":"Philippe
Oberti","email":"philippe.oberti@elastic.co"},"sourceCommit":{"committedDate":"2024-09-20T00:39:28Z","message":"[Security
Solution][Notes] - add note block to alert defaitls flyout header
(#193373)","sha":"258adf533561b11d0cc0dae8c2fc2a1acc0d8e02","branchLabelMapping":{"^v9.0.0$":"main","^v8.16.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","v9.0.0","Team:Threat
Hunting:Investigations","backport:prev-minor","v8.16.0"],"title":"[Security
Solution][Notes] - add note block to alert defaitls flyout
header","number":193373,"url":"https://github.com/elastic/kibana/pull/193373","mergeCommit":{"message":"[Security
Solution][Notes] - add note block to alert defaitls flyout header
(#193373)","sha":"258adf533561b11d0cc0dae8c2fc2a1acc0d8e02"}},"sourceBranch":"main","suggestedTargetBranches":["8.x"],"targetPullRequestStates":[{"branch":"main","label":"v9.0.0","branchLabelMappingKey":"^v9.0.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/193373","number":193373,"mergeCommit":{"message":"[Security
Solution][Notes] - add note block to alert defaitls flyout header
(#193373)","sha":"258adf533561b11d0cc0dae8c2fc2a1acc0d8e02"}},{"branch":"8.x","label":"v8.16.0","branchLabelMappingKey":"^v8.16.0$","isSourceBranch":false,"state":"NOT_CREATED"}]}]
BACKPORT-->

Co-authored-by: Philippe Oberti <philippe.oberti@elastic.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) release_note:skip Skip the PR/issue when compiling release notes Team:Threat Hunting:Investigations Security Solution Investigations Team v8.16.0 v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants