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] - remove old flyout unnecessary z-index change #181480

Merged
merged 1 commit into from
Apr 25, 2024

Conversation

PhilippeOberti
Copy link
Contributor

@PhilippeOberti PhilippeOberti commented Apr 23, 2024

Summary

A big was introduced over the last few weeks, where the old alert details flyout is now shown in front of the timeline, instead of behind.
I'm not sure exactly why the z-index value was changed to 1002 in this recent PR, but the timeline z-index is set to 1001, so the flyout would always be shown above. The changes to the timeline z-index where performed in this PR.

@michaelolo24 do you know why this recent change was made? Can you help me test if you remember the scenarios that made you want to make the change in the first place?

This PR removes that z-index override, and the issue is fixed (at least in the testing I've done).

Before fix

Screen.Recording.2024-04-23.at.12.50.06.PM.mov

After fix

Screen.Recording.2024-04-23.at.12.56.22.PM.mov

#180679

@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 Apr 23, 2024
@PhilippeOberti PhilippeOberti requested a review from a team as a code owner April 23, 2024 18:05
@elasticmachine
Copy link
Contributor

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

@@ -55,7 +57,8 @@ describe(
cy.get(GET_DISCOVER_DATA_GRID_CELL_HEADER('agent.type')).should('not.exist');
});

context('flyout', () => {
// these tests are skipped until we implement the expandable flyout in the unified table for timeline
context.skip('flyout', () => {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@michaelolo24 let me know if you're ok skipping these tests. Explanation of why I think we should do this here

Copy link
Contributor

Choose a reason for hiding this comment

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

Yea, that's fine. I think the flyout in timeline being enabled will align with when the unified table is ready so no worries, we can move skip this. Fyi @logeekal

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 introducing this fix!

@PhilippeOberti PhilippeOberti force-pushed the old-flyout-z-index branch 3 times, most recently from ca09c4c to 50ad661 Compare April 25, 2024 18:08
@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] FTR Configs #64 / Monitoring Endpoints APM "before all" hook in "APM"

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 17.3MB 17.3MB -200.0B

History

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

@PhilippeOberti PhilippeOberti merged commit 84e3ea5 into elastic:main Apr 25, 2024
35 checks passed
@PhilippeOberti PhilippeOberti deleted the old-flyout-z-index branch April 25, 2024 21:39
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Apr 25, 2024
@kibanamachine
Copy link
Contributor

💚 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

kibanamachine added a commit that referenced this pull request Apr 25, 2024
…sary z-index change (#181480) (#181784)

# Backport

This will backport the following commits from `main` to `8.14`:
- [[Security Solution][Alert details] - remove old flyout unnecessary
z-index change (#181480)](#181480)

<!--- 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-04-25T21:39:50Z","message":"[Security
Solution][Alert details] - remove old flyout unnecessary z-index change
(#181480)","sha":"84e3ea51a8f5b8ead0abfcf40016c02d0b3c3ab2","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"],"title":"[Security
Solution][Alert details] - remove old flyout unnecessary z-index
change","number":181480,"url":"#181480
Solution][Alert details] - remove old flyout unnecessary z-index change
(#181480)","sha":"84e3ea51a8f5b8ead0abfcf40016c02d0b3c3ab2"}},"sourceBranch":"main","suggestedTargetBranches":["8.14"],"targetPullRequestStates":[{"branch":"8.14","label":"v8.14.0","branchLabelMappingKey":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.15.0","branchLabelMappingKey":"^v8.15.0$","isSourceBranch":true,"state":"MERGED","url":"#181480
Solution][Alert details] - remove old flyout unnecessary z-index change
(#181480)","sha":"84e3ea51a8f5b8ead0abfcf40016c02d0b3c3ab2"}}]}]
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
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.

None yet

5 participants