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][Alert details] - fix timeline ES|QL flyout shown behind #182296

Merged

Conversation

PhilippeOberti
Copy link
Contributor

@PhilippeOberti PhilippeOberti commented May 1, 2024

Summary

This PR addresses an issue where the flyout for the unified data table used in the ES|QL tab of Timeline is shown behind timeline. This was most likely introduced when we merged this PR that introduced the expandable flyout to timeline, where we had to modify the z-indexof timeline from 1000 to 1001. Therefore all flyouts opened from timeline should have their z-index set to higher or equal to 1002.

The solution proposed in the PR is not ideal. I wish there was a way to use our own flyout (as we should be using the expandable flyout for UI consistency in Security Solution).
Or at least I wish we could pass some properties down to the flyout to render it differently from how it is done in Discover at the moment (maybe there is a way but I'm not aware of it).
The code was updated to have the logic directly within the SecuritySolution plugin instead of within Discover. Therefore there aren't any impact on the Discover app!

As 8.14 is upon us, I felt like this was a simple, non-intrusive (hopefully) solution.

Before fix (flyout is open but hidden behind timeline)
Screenshot 2024-05-01 at 3 12 36 PM

After fix
Screenshot 2024-05-01 at 3 11 53 PM

#180106

@PhilippeOberti PhilippeOberti added release_note:skip Skip the PR/issue when compiling release notes Team:Threat Hunting:Investigations Security Solution Investigations Team v8.14.0 labels May 1, 2024
@PhilippeOberti PhilippeOberti requested a review from a team as a code owner May 1, 2024 20:19
@elasticmachine
Copy link
Contributor

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

Copy link
Contributor

@logeekal logeekal left a comment

Choose a reason for hiding this comment

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

Just left one comment.

@PhilippeOberti PhilippeOberti force-pushed the discover-table-timeline-flyout branch from 7fb53ed to a1cf66f Compare May 2, 2024 16:33
@PhilippeOberti PhilippeOberti requested a review from a team as a code owner May 2, 2024 16:33
Copy link
Contributor

@logeekal logeekal left a comment

Choose a reason for hiding this comment

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

Thanks @PhilippeOberti for making these changes.


// TODO remember to remove the className added to discover/public/components/discover_grid_flyout/discover_grid_flyout.tsx when removing this
export const TimelineESQLGlobalStyles = createGlobalStyle`
body:has(.timeline-portal-overlay-mask) .DiscoverFlyout {
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks! I noticed it was .euiFlyout before and was wondering how that affected the other flyouts 😅 . Nice work, tested and works great!

Copy link
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.

Thanks for getting this fix in. LGTM! 🎉

@PhilippeOberti PhilippeOberti enabled auto-merge (squash) May 2, 2024 17:07
@kibana-ci
Copy link
Collaborator

💚 Build Succeeded

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
discover 637.3KB 637.3KB +27.0B
securitySolution 13.7MB 13.7MB +135.0B
total +162.0B

Saved Objects .kibana field count

Every field in each saved object type adds overhead to Elasticsearch. Kibana needs to keep the total field count below Elasticsearch's default limit of 1000 fields. Only specify field mappings for the fields you wish to search on or query. See https://www.elastic.co/guide/en/kibana/master/saved-objects-service.html#_mappings

id before after diff
_ignored_source 1 - -1

History

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

@PhilippeOberti PhilippeOberti merged commit 2c6d13e into elastic:main May 2, 2024
36 checks passed
@PhilippeOberti PhilippeOberti deleted the discover-table-timeline-flyout branch May 2, 2024 21:53
@PhilippeOberti PhilippeOberti restored the discover-table-timeline-flyout branch May 2, 2024 21:54
@PhilippeOberti PhilippeOberti deleted the discover-table-timeline-flyout branch May 2, 2024 21:55
@kibanamachine
Copy link
Contributor

💔 All backports failed

Status Branch Result
8.14 Backport failed because of merge conflicts

Manual backport

To create the backport manually run:

node scripts/backport --pr 182296

Questions ?

Please refer to the Backport tool documentation

@PhilippeOberti
Copy link
Contributor Author

💚 All backports created successfully

Status Branch Result
8.14

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

Questions ?

Please refer to the Backport tool documentation

PhilippeOberti added a commit to PhilippeOberti/kibana that referenced this pull request May 2, 2024
…behind (elastic#182296)

(cherry picked from commit 2c6d13e)

# Conflicts:
#	src/plugins/discover/public/components/discover_grid_flyout/discover_grid_flyout.tsx
PhilippeOberti added a commit that referenced this pull request May 2, 2024
… shown behind (#182296) (#182529)

# Backport

This will backport the following commits from `main` to `8.14`:
- [[Security Solution][Alert details] - fix timeline ES|QL flyout shown
behind (#182296)](#182296)

<!--- Backport version: 8.9.8 -->

### 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-05-02T21:51:52Z","message":"[Security
Solution][Alert details] - fix timeline ES|QL flyout shown behind
(#182296)","sha":"2c6d13e661db96988e51a355dbcbde5e6d04e1e1","branchLabelMapping":{"^v8.15.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","Team:Threat
Hunting:Investigations","v8.14.0","v8.15.0"],"number":182296,"url":"https://github.com/elastic/kibana/pull/182296","mergeCommit":{"message":"[Security
Solution][Alert details] - fix timeline ES|QL flyout shown behind
(#182296)","sha":"2c6d13e661db96988e51a355dbcbde5e6d04e1e1"}},"sourceBranch":"main","suggestedTargetBranches":["8.14"],"targetPullRequestStates":[{"branch":"8.14","label":"v8.14.0","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.15.0","labelRegex":"^v8.15.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/182296","number":182296,"mergeCommit":{"message":"[Security
Solution][Alert details] - fix timeline ES|QL flyout shown behind
(#182296)","sha":"2c6d13e661db96988e51a355dbcbde5e6d04e1e1"}}]}]
BACKPORT-->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release_note:skip Skip the PR/issue when compiling release notes Team:Threat Hunting:Investigations Security Solution Investigations Team v8.14.0 v8.15.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants