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

[Defend Workflows][Bug] Case flyout z-index #153219

Merged
merged 37 commits into from
May 3, 2023

Conversation

szwarckonrad
Copy link
Contributor

@szwarckonrad szwarckonrad commented Mar 15, 2023

Fixes https://github.com/elastic/security-team/issues/6228

5000 z-index set in create-case-flyout-mask-overlay is being overwritten by euiOverlayMask-belowHeader with a value of 1000. This causes Case flyout to be under the already opened Osquery flyout

This PR includes cleanup in flyouts renders - we removed unnecessary maskProps as well as z-indexes that were explicitly set even though flyout component manages them itself.

test

@szwarckonrad szwarckonrad added bug Fixes for quality problems that affect the customer experience Team:Defend Workflows “EDR Workflows” sub-team of Security Solution Feature:Osquery Security Solution Osquery feature v8.7.0 v8.8.0 release_note:skip Skip the PR/issue when compiling release notes labels Mar 15, 2023
@szwarckonrad szwarckonrad changed the title [Bug] Case flyout z-index [Defend Workflows][Bug] Case flyout z-index Mar 15, 2023
@szwarckonrad szwarckonrad requested review from tomsonpl and patrykkopycinski and removed request for patrykkopycinski March 15, 2023 15:13
@szwarckonrad szwarckonrad marked this pull request as ready for review March 15, 2023 15:18
@szwarckonrad szwarckonrad requested review from a team as code owners March 15, 2023 15:18
@szwarckonrad szwarckonrad requested a review from parkiino March 15, 2023 15:18
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-defend-workflows (Team:Defend Workflows)

Copy link
Contributor

@tomsonpl tomsonpl left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@jonathan-buttner jonathan-buttner added ci:cloud-deploy Create or update a Cloud deployment ci:cloud-redeploy Always create a new Cloud deployment labels Mar 21, 2023
Copy link
Contributor

@jonathan-buttner jonathan-buttner 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 taking a look at the issue. It looks like the changes have affected how the global nav header is shown. With the new changes the global nav header shows up behind the flyout and is grayed out where as previously it was not grayed out.

Here's with the changes

Screenshot 2023-03-21 at 11 40 29 AM

Here's how the flyout looked without the z-index changes
image

@tomsonpl
Copy link
Contributor

@jonathan-buttner do you think it's actually a bad thing? :P why should we see a navbar while using a modal?

@jonathan-buttner
Copy link
Contributor

@jonathan-buttner do you think it's actually a bad thing? :P why should we see a navbar while using a modal?

Yeah I think we want the header to be selectable while using a flyout. That's consistent with how the rest of Kibana works

Security solution exampleimage

Discover example

image

@tomsonpl
Copy link
Contributor

tomsonpl commented Mar 22, 2023

@jonathan-buttner Ok it should be fine now 👍 we changed above to below, and the issue is still fixed :elastic-heart:

@maximpn
Copy link
Contributor

maximpn commented May 2, 2023

This PR fixes this bug #156147. The fix is really crucial to get in 8.8.

@patrykkopycinski patrykkopycinski added ci:skip-cypress-osquery Skips osquery cypress checks and removed ci:cloud-deploy Create or update a Cloud deployment ci:cloud-redeploy Always create a new Cloud deployment labels May 2, 2023
@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
securitySolution 3859 3860 +1

Async chunks

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

id before after diff
cases 376.9KB 376.7KB -190.0B
securitySolution 9.1MB 9.1MB -818.0B
total -1008.0B
Unknown metric groups

ESLint disabled line counts

id before after diff
enterpriseSearch 19 21 +2
securitySolution 398 401 +3
total +5

Total ESLint disabled count

id before after diff
enterpriseSearch 20 22 +2
securitySolution 478 481 +3
total +5

History

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

@@ -37,7 +37,6 @@ const MyEuiModal = styled(EuiModal)`
height: auto !important;
max-width: 718px;
}
z-index: 99999999;
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure, how to test this case

Copy link
Contributor

@nkhristinin nkhristinin left a comment

Choose a reason for hiding this comment

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

@patrykkopycinski patrykkopycinski merged commit d8fe39c into elastic:main May 3, 2023
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request May 3, 2023
Fixes elastic/security-team#6228

5000 `z-index` set in `create-case-flyout-mask-overlay` is being
overwritten by `euiOverlayMask-belowHeader` with a value of 1000. This
causes **Case flyout** to be under the already opened **Osquery flyout**

This PR includes cleanup in flyouts renders - we removed unnecessary
`maskProps` as well as z-indexes that were explicitly set even though
flyout component manages them itself.

![test](https://user-images.githubusercontent.com/29123534/225292177-a08d3fb8-aad3-487b-a054-6cde6aec94d7.gif)

---------

Co-authored-by: Tomasz Ciecierski <[email protected]>
Co-authored-by: Tomasz Ciecierski <[email protected]>
Co-authored-by: Patryk Kopyciński <[email protected]>
Co-authored-by: kibanamachine <[email protected]>
(cherry picked from commit d8fe39c)
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
8.8

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

Questions ?

Please refer to the Backport tool documentation

kevinlog pushed a commit that referenced this pull request May 3, 2023
# Backport

This will backport the following commits from `main` to `8.8`:
- [[Defend Workflows][Bug] Case flyout z-index
(#153219)](#153219)

<!--- Backport version: 8.9.7 -->

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

<!--BACKPORT [{"author":{"name":"Konrad
Szwarc","email":"[email protected]"},"sourceCommit":{"committedDate":"2023-05-03T10:30:13Z","message":"[Defend
Workflows][Bug] Case flyout z-index (#153219)\n\nFixes
https://github.com/elastic/security-team/issues/6228\r\n\r\n5000
`z-index` set in `create-case-flyout-mask-overlay` is
being\r\noverwritten by `euiOverlayMask-belowHeader` with a value of
1000. This\r\ncauses **Case flyout** to be under the already opened
**Osquery flyout**\r\n\r\nThis PR includes cleanup in flyouts renders -
we removed unnecessary\r\n`maskProps` as well as z-indexes that were
explicitly set even though\r\nflyout component manages them
itself.\r\n\r\n\r\n![test](https://user-images.githubusercontent.com/29123534/225292177-a08d3fb8-aad3-487b-a054-6cde6aec94d7.gif)\r\n\r\n---------\r\n\r\nCo-authored-by:
Tomasz Ciecierski <[email protected]>\r\nCo-authored-by: Tomasz
Ciecierski <[email protected]>\r\nCo-authored-by: Patryk
Kopyciński <[email protected]>\r\nCo-authored-by:
kibanamachine
<[email protected]>","sha":"d8fe39c18d9df7acb00a82515f5a41a6e4111c59","branchLabelMapping":{"^v8.9.0$":"main","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["bug","release_note:skip","Team:Defend
Workflows","Feature:Osquery","ci:all-cypress-suites","v8.8.0","ci:skip-cypress-osquery","v8.9.0"],"number":153219,"url":"https://github.com/elastic/kibana/pull/153219","mergeCommit":{"message":"[Defend
Workflows][Bug] Case flyout z-index (#153219)\n\nFixes
https://github.com/elastic/security-team/issues/6228\r\n\r\n5000
`z-index` set in `create-case-flyout-mask-overlay` is
being\r\noverwritten by `euiOverlayMask-belowHeader` with a value of
1000. This\r\ncauses **Case flyout** to be under the already opened
**Osquery flyout**\r\n\r\nThis PR includes cleanup in flyouts renders -
we removed unnecessary\r\n`maskProps` as well as z-indexes that were
explicitly set even though\r\nflyout component manages them
itself.\r\n\r\n\r\n![test](https://user-images.githubusercontent.com/29123534/225292177-a08d3fb8-aad3-487b-a054-6cde6aec94d7.gif)\r\n\r\n---------\r\n\r\nCo-authored-by:
Tomasz Ciecierski <[email protected]>\r\nCo-authored-by: Tomasz
Ciecierski <[email protected]>\r\nCo-authored-by: Patryk
Kopyciński <[email protected]>\r\nCo-authored-by:
kibanamachine
<[email protected]>","sha":"d8fe39c18d9df7acb00a82515f5a41a6e4111c59"}},"sourceBranch":"main","suggestedTargetBranches":["8.8"],"targetPullRequestStates":[{"branch":"8.8","label":"v8.8.0","labelRegex":"^v(\\d+).(\\d+).\\d+$","isSourceBranch":false,"state":"NOT_CREATED"},{"branch":"main","label":"v8.9.0","labelRegex":"^v8.9.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/153219","number":153219,"mergeCommit":{"message":"[Defend
Workflows][Bug] Case flyout z-index (#153219)\n\nFixes
https://github.com/elastic/security-team/issues/6228\r\n\r\n5000
`z-index` set in `create-case-flyout-mask-overlay` is
being\r\noverwritten by `euiOverlayMask-belowHeader` with a value of
1000. This\r\ncauses **Case flyout** to be under the already opened
**Osquery flyout**\r\n\r\nThis PR includes cleanup in flyouts renders -
we removed unnecessary\r\n`maskProps` as well as z-indexes that were
explicitly set even though\r\nflyout component manages them
itself.\r\n\r\n\r\n![test](https://user-images.githubusercontent.com/29123534/225292177-a08d3fb8-aad3-487b-a054-6cde6aec94d7.gif)\r\n\r\n---------\r\n\r\nCo-authored-by:
Tomasz Ciecierski <[email protected]>\r\nCo-authored-by: Tomasz
Ciecierski <[email protected]>\r\nCo-authored-by: Patryk
Kopyciński <[email protected]>\r\nCo-authored-by:
kibanamachine
<[email protected]>","sha":"d8fe39c18d9df7acb00a82515f5a41a6e4111c59"}}]}]
BACKPORT-->

Co-authored-by: Konrad Szwarc <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Fixes for quality problems that affect the customer experience ci:all-cypress-suites ci:skip-cypress-osquery Skips osquery cypress checks Feature:Osquery Security Solution Osquery feature release_note:skip Skip the PR/issue when compiling release notes Team:Defend Workflows “EDR Workflows” sub-team of Security Solution v8.8.0 v8.9.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.