[Observability][SecuritySolution] Fix to prevent observability style conflict in flyout across plugins #138091

logeekal · 2022-08-04T10:21:15Z

Problem & Fix

Hello @elastic/observability-ui & @elastic/security-threat-hunting-investigations team,

Code below was creating conflict with the styles of the Flyout components in Security Solution -> Alerts since, it only uses EUI standard classes. I think it could easily conflict with styles of other plugins as well. For more details please check the bug mentioned above.

kibana/x-pack/plugins/observability/public/pages/alerts/containers/alerts_page/styles.scss

Lines 6 to 10 in bd2361d

    
           .euiFlyout, 
        
           .euiCollapsibleNav { 
        
             top: $fullscreenFlyoutTop; 
        
             height: calc(100% - #{$fullscreenFlyoutTop});

I could not find any observability specific class tokens so I have taken a liberty to add one. Please feel free to suggest to push any change that you may seem correct.

Suggestion

Ideally, I think it will be better if there a common wrapper div section around observability pages that can have an oblt class token and this way all the styles can be specific to observability plugin.

This model can be applied to all the plugins for style segregation.

Caveats

This error only comes to light when user has visited both observability plugin and security solution plugin. This makes the situation difficult to identify and test. There is saperate github issue : #138102 to resolve this and you can contribute there if you have anything to add regarding how styles are loaded in kibana.

elasticmachine · 2022-08-04T10:45:42Z

Pinging @elastic/unified-observability (Team:Observability)

janmonschke · 2022-08-04T12:34:12Z

x-pack/plugins/observability/public/pages/alerts/components/alerts_flyout/alerts_flyout.tsx

@@ -45,7 +45,12 @@ export function AlertsFlyout({
  }

  return (
-    <EuiFlyout onClose={onClose} size="s" data-test-subj="alertsFlyout">
+    <EuiFlyout
+      className="oblt__alert--flyout oblt__flyout"


Is oblt__alert--flyout used anywhere?

No, I will remove it. I started with this and then added more generic oblt__flyout, but forgot to remove this.

janmonschke · 2022-08-04T12:35:14Z

x-pack/plugins/observability/public/pages/alerts/containers/alerts_page/styles.scss

+  .euiFlyout.oblt__flyout,
  .euiCollapsibleNav {


It might make sense to scope euiCollapsibleNav to oblt__flyout as well, wdyt?

I thought so as well but I could not find any instance of EuiCollapsibleNav being used in Observability so I left it alone.

Okay I found that EuiCollapsible is the left sidebar collapsible Menu and this CSS applies to Collapsible menu only and only when Data grid is full screen. But when datagrid is full screen, there is no way to open the collapsible menu. See below screenshot.

So, I guess it will be okay, we do not touch it. because I delete .euiCollapsibleNav from here, not sure what will break.

fkanout

Actionable Observability changes LGTM. Tested locally and the flyout worked as expected.

kibana-ci · 2022-08-09T11:09:56Z

💚 Build Succeeded

Buildkite Build
Commit: 55811f2

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
`observability`	504.5KB	504.6KB	+76.0B

History

💚 Build #63108 succeeded 84ac2ee

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

jasonrhodes

Hmm, it seems to me that for localized styles we should be using something other than hand-rolled class names and relying on the CSS cascade. Let me check with our design team to see how we usually handle this and then get back to you ASAP.

logeekal · 2022-08-10T14:16:50Z

Thanks @jasonrhodes for looking into it.

jasonrhodes

OK so I've asked around and I think we need to remove these global styles altogether and replace them with something more localized. That said, to unblock you all, I think we can merge the changes as-is and then file a follow up ticket to replace them (with emotion syntax, likely).

Since @fkanout has already approved it from the perspective of the @elastic/actionable-observability team (which owns the alerts page), I'll approve too.

Thanks for your patience!

logeekal · 2022-08-10T14:47:52Z

Great. Thank you.. emotion makes sense to modularize styles.

Do you know who can approve on behalf of @elastic/observability-design as it is being shown as the code owner?

formgeist

LGTM

kibanamachine · 2022-08-10T16:00:56Z

💚 All backports created successfully

Status	Branch	Result
✅	8.4

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

Questions ?

Please refer to the Backport tool documentation

…conflict in flyout across plugins (elastic#138091) * fix to prevent style conflict in flyouts from diff. plugins * remove unnecessary class token (cherry picked from commit 3aa0599)

…conflict in flyout across plugins (elastic#138091) (elastic#138533) * fix to prevent style conflict in flyouts from diff. plugins * remove unnecessary class token (cherry picked from commit 3aa0599) Co-authored-by: Jatin Kathuria <jtn.kathuria@gmail.com>

fix to prevent style conflict in flyouts from diff. plugins

84ac2ee

logeekal added the backport:prev-major Backport to (8.x, 8.16, 8.15) the previous major branch and all later branches still in development label Aug 4, 2022

logeekal requested review from a team August 4, 2022 10:21

logeekal added backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) and removed backport:prev-major Backport to (8.x, 8.16, 8.15) the previous major branch and all later branches still in development labels Aug 4, 2022

logeekal marked this pull request as ready for review August 4, 2022 10:36

logeekal requested review from a team as code owners August 4, 2022 10:36

logeekal added Team:Observability Team label for Observability Team (for things that are handled across all of observability) Team:Threat Hunting:Investigations Security Solution Investigations Team labels Aug 4, 2022

logeekal added the release_note:skip Skip the PR/issue when compiling release notes label Aug 4, 2022

logeekal mentioned this pull request Aug 4, 2022

[Core] Clarity regarding bundling and loading of different plugins and their styles #138102

Closed

janmonschke reviewed Aug 4, 2022

View reviewed changes

fkanout self-requested a review August 4, 2022 13:59

fkanout approved these changes Aug 4, 2022

View reviewed changes

logeekal added the v8.5.0 label Aug 4, 2022

remove unnecessary class token

3d5c148

janmonschke approved these changes Aug 9, 2022

View reviewed changes

Merge branch 'main' into fix/oblt_security_flyout_style_conflict

55811f2

logeekal removed the request for review from a team August 9, 2022 10:06

formgeist requested a review from jasonrhodes August 9, 2022 11:01

jasonrhodes reviewed Aug 10, 2022

View reviewed changes

jasonrhodes approved these changes Aug 10, 2022

View reviewed changes

jasonrhodes mentioned this pull request Aug 10, 2022

[Observability] Replace global styles with emotion syntax for Alerts Page #138526

Closed

logeekal enabled auto-merge (squash) August 10, 2022 14:52

formgeist approved these changes Aug 10, 2022

View reviewed changes

logeekal merged commit 3aa0599 into main Aug 10, 2022

logeekal deleted the fix/oblt_security_flyout_style_conflict branch August 10, 2022 15:57

kibanamachine mentioned this pull request Aug 10, 2022

[8.4] [Observability][SecuritySolution] Fix to prevent observability style conflict in flyout across plugins (#138091) #138533

Merged

kibanamachine added the v8.4.0 label Aug 10, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Observability][SecuritySolution] Fix to prevent observability style conflict in flyout across plugins #138091

[Observability][SecuritySolution] Fix to prevent observability style conflict in flyout across plugins #138091

logeekal commented Aug 4, 2022 •

edited by kibanamachine

Loading

elasticmachine commented Aug 4, 2022

janmonschke Aug 4, 2022

logeekal Aug 4, 2022

janmonschke Aug 4, 2022

logeekal Aug 4, 2022

logeekal Aug 9, 2022

fkanout left a comment

kibana-ci commented Aug 9, 2022

jasonrhodes left a comment

logeekal commented Aug 10, 2022

jasonrhodes left a comment

logeekal commented Aug 10, 2022

formgeist left a comment

kibanamachine commented Aug 10, 2022


	.euiFlyout,
	.euiCollapsibleNav {
	top: $fullscreenFlyoutTop;
	height: calc(100% - #{$fullscreenFlyoutTop});

[Observability][SecuritySolution] Fix to prevent observability style conflict in flyout across plugins #138091

[Observability][SecuritySolution] Fix to prevent observability style conflict in flyout across plugins #138091

Conversation

logeekal commented Aug 4, 2022 • edited by kibanamachine Loading

Problem & Fix

Suggestion

Caveats

elasticmachine commented Aug 4, 2022

janmonschke Aug 4, 2022

Choose a reason for hiding this comment

logeekal Aug 4, 2022

Choose a reason for hiding this comment

janmonschke Aug 4, 2022

Choose a reason for hiding this comment

logeekal Aug 4, 2022

Choose a reason for hiding this comment

logeekal Aug 9, 2022

Choose a reason for hiding this comment

fkanout left a comment

Choose a reason for hiding this comment

kibana-ci commented Aug 9, 2022

💚 Build Succeeded

Metrics [docs]

Async chunks

History

jasonrhodes left a comment

Choose a reason for hiding this comment

logeekal commented Aug 10, 2022

jasonrhodes left a comment

Choose a reason for hiding this comment

logeekal commented Aug 10, 2022

formgeist left a comment

Choose a reason for hiding this comment

kibanamachine commented Aug 10, 2022

💚 All backports created successfully

Questions ?

logeekal commented Aug 4, 2022 •

edited by kibanamachine

Loading