[Security Solution][Detection Engine] EUI Tech Debt - Removes references to static EuiTheme variables by rylnd · Pull Request #208820 · elastic/kibana

rylnd · 2025-01-29T18:17:08Z

Summary

This PR is a followup to #205990, which removed references to all of the deprecated/renamed EUI vars in preparation for 9.0. Here, we address some of the non-critical tech debt related to the EUI refresh, namely the removal of static EUI tokens from our codebase.

I made every attempt not to change any styles in this PR, except to simplify CSS to produce an equivalent design. A common example of this was removing a static margin or padding declaration referencing euiThemeVars.size*, and swapping it with an equivalent gutterSize prop on the EuiFlexGroup container, or with an align-self or other equivalent flexbox directive.

Screenshots of Areas Affected

The majority of changes here involved the Exception List/Item pages. I've attached screenshots of their current layout for comparison/review:

Rule Exceptions Tab

Before After

Shared Exception Lists

Shared Exception List Details

Threshold Input

Checklist

Check the PR satisfies following conditions.

Reviewers should verify this PR satisfies this list as well.

Unit or functional tests were updated or added to match the most common scenarios
The PR description includes the appropriate Release Notes section, and the correct release_note:* label is applied per the guidelines

rylnd · 2025-01-29T18:17:19Z

/ci

rylnd · 2025-01-29T20:52:12Z

Note to self: I think that 7b5441e was a mistake. That function should reference components that use the useEuiTheme() hook, as opposed to calling the hook directly.

The EUI page header has some parent <p> somewhere, and we were incorrectly putting <div>s as children. This fixes that, along with a few necessary style changes (and updated snapshots).

This replaces static references to euiThemeVars to enable this behavior. I did away with some `margin` usage in favor of `EuiFlexGroup`'s `gutterSize` prop, as well as removing some manual `display: flex`s in favor of EuiFlexItem/Group. This also appears to fix some errors in the snapshot test related to this component. I'm not sure how those happened, but it appears specific to that test and not a production behavior.

While removing static references to euiThemeVars, I found that these manual styles weren't needed: * `&div:first-child` selector wasn't being applied * margins can be replaced with a different `gutterSize` prop * the eui-yScrollWithShadow was adding a shadow to a container that wasn't actually scrollable; when the extra margin above was removed, this caused the shadow to cover part of the border of the Exception item panels

We had negative-margin CSS to offset a 24px `gap` on our flex grouping. By reducing the `gap` to 8px, we no longer need to offset it.

Instead of adding bottom padding to each element in a container (the header of an individual exception list on its Shared Details page), we instead make the container a `FlexGroup` and add an analogous `gutterSize` prop (and `margin-bottom`) to it.

I forgot about the aforementioned weirdness in EUIPageHeader that necessitates everything being a span.

We were wrapping a `span` in a `div`, and then giving that div: 1. display: inline 2. margin-left: 4px Because the margin is so insignificant (there is already natural space after the previous element), I opted to just remove the div and its styles altogether.

The generated styles are equivalent.

…tCard This component exists as the inner item on the Shared Exception Lists page. Styles have been verified as equivalent.

* Defines a FieldSectionGroup component instead of applying the same styles to multiple identical EuiFieldGroup components * Replaces use of margin-top with a more appropriate `align-self` declaration

…apping'

These were already fixed on the package version of this component, but the one that's still in the plugin was not updated, which I noticed while taking screenshots.

This ports the styles over from static generation with euiThemeVars to dynamic with useEuiTheme. I also simplified styles in a few ways, here: 1. Remove the JS conditions around applying styles, and porting to equivalent CSS 2. Removing unnecessary flex-basis CSS (which didn't work on the div it was being applied to) 3. Moving child `color` CSS to the parent, instead.

These tests were failing because they lacked the EUI Theme context when we were rendering these components which now require them (previously they were requiring a static object imported from elsewhere).

rylnd · 2025-01-31T23:48:01Z

/ci

We don't really want to test the EuiThemeProvider, it's merely a consequence of the component's dependencies/how it's written. Calling the provider out as a wrapper better helps to show that relationship.

rylnd · 2025-02-01T04:26:32Z

/ci

elasticmachine · 2025-02-01T18:07:37Z

Pinging @elastic/security-detection-engine (Team:Detection Engine)

vitaliidm · 2025-02-03T15:39:44Z

...lutions/security/plugins/security_solution/public/value_list/components/value_list_modal.tsx

 */
 import React, { useState, useCallback } from 'react';
 import { css } from '@emotion/css';
+import styled from '@emotion/styled';


shouldn't we avoid using styled components according to elastic/eui#6828 (comment) and give preference to @emotion/css or @emotion/react?

@vitaliidm this is not the incompatible styled-components package but rather a variant of emotion's syntax. It's mentioned later in the comment you referenced:

As a simple first step in this transition, consider using @emotion/styled. This allows you to keep the familiar styled syntax while aligning with the Emotion ecosystem

It's unclear whether we'll need to change these in the future, but if so we can address them similarly/concurrently with the removal of remaining styled-components references.

Yes, it's part of@emotion, but what got me curious - why the need to introduce styled in this file and couple of others?

We did not have styled-components here at all, so using it @emotion/styled as a transition step does not apply in this case.
In x-pack/solutions/security/plugins/security_solution/public/exceptions/pages/shared_lists/index.tsx for example, styled-components were replaced by @emotion/styled and I see why you did it there. It's in line with the eui-team guide.

elasticmachine · 2025-02-04T21:28:58Z

💚 Build Succeeded

Buildkite Build
Commit: 3df4d2d

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id	before	after	diff
`securitySolution`	6656	6654	-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`	21.5MB	21.5MB	-3.8KB

History

💔 Build #272507 failed 7684a34
💔 Build #272489 failed eca40cd
💔 Build #272469 failed eb60210
💔 Build #271730 failed 61f81a2b9c7bea80bb7e3b7fd49f972a1fc4b903

cc @rylnd

vitaliidm

Changes LGTM. Thanks for addressing tech debt

…ces to static EuiTheme variables (elastic#208820) ## Summary This PR is a followup to elastic#205990, which removed references to all of the deprecated/renamed EUI vars in preparation for 9.0. Here, we address some of the non-critical tech debt related to the EUI refresh, namely the [removal of static EUI tokens](elastic#199715 (comment)) from our codebase. I made every attempt not to change any styles in this PR, except to simplify CSS to produce an equivalent design. A common example of this was removing a static `margin` or `padding` declaration referencing `euiThemeVars.size*`, and swapping it with an equivalent `gutterSize` prop on the `EuiFlexGroup` container, or with an `align-self` or other equivalent flexbox directive. ## Screenshots of Areas Affected The majority of changes here involved the Exception List/Item pages. I've attached screenshots of their current layout for comparison/review: <details> <summary> <h3>Rule Exceptions Tab</h3> </summary> <kbd> <h3>Before</h3> <img width="1395" alt="Rule exceptions tab - before" src="https://github.com/user-attachments/assets/db7a5487-7df3-4a5f-b88e-90ab34784970" /> </kbd> <kbd> <h3>After</h3> <img width="1421" alt="Rule exceptions tab - after" src="https://github.com/user-attachments/assets/77cbdefc-cbec-4b9e-8436-197f2f2f6677" /> </kbd> </details> <details> <summary> <h3>Shared Exception Lists</h3> </summary> <kbd> <img width="1517" alt="Shared Exception Lists" src="https://github.com/user-attachments/assets/5448dd65-60f7-470c-bf7a-7af75bb914fa" /> </kbd> </details> <details> <summary> <h3>Shared Exception List Details</h3> </summary> <kbd> <img width="1517" alt="Shared Exception List Details" src="https://github.com/user-attachments/assets/6e03fef6-af58-40bb-98c5-188651a584cc" /> </kbd> </details> <details> <summary> <h3>Threshold Input</h3> </summary> <kbd> <img width="1046" alt="Threshold Input" src="https://github.com/user-attachments/assets/28738857-6bdf-404f-a790-a9f4e66ff27a" /> </kbd> </details> ### Checklist Check the PR satisfies following conditions. Reviewers should verify this PR satisfies this list as well. - [x] [Unit or functional tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html) were updated or added to match the most common scenarios - [x] The PR description includes the appropriate Release Notes section, and the correct `release_note:*` label is applied per the [guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process) --------- Co-authored-by: kibanamachine <42973632+kibanamachine@users.noreply.github.com>

rylnd added release_note:skip Skip the PR/issue when compiling release notes backport:skip This PR does not require backporting Team:Detection Engine Security Solution Detection Engine Area EUI Visual Refresh labels Jan 29, 2025

rylnd self-assigned this Jan 29, 2025

rylnd and others added 21 commits January 31, 2025 16:31

Fix incorrect DOM nesting on ExceptionListHeader component

11f529f

The EUI page header has some parent <p> somewhere, and we were incorrectly putting <div>s as children. This fixes that, along with a few necessary style changes (and updated snapshots).

Remove euiThemeVars from Empty View component

89a7dfb

Make font-family styles dynamic with theme

708a32f

Replace custom CSS with appropriate gutterSize prop

b729007

We had negative-margin CSS to offset a 24px `gap` on our flex grouping. By reducing the `gap` to 8px, we no longer need to offset it.

Update snapshots

85935a6

Fix DOM nesting issue by using span for flexgroup

dce42a8

I forgot about the aforementioned weirdness in EUIPageHeader that necessitates everything being a span.

Remove unnecessary styles

c325a0c

We were wrapping a `span` in a `div`, and then giving that div: 1. display: inline 2. margin-left: 4px Because the margin is so insignificant (there is already natural space after the previous element), I opted to just remove the div and its styles altogether.

Convert MlJobItem to use @emotion and dynamic vars

134bc93

The generated styles are equivalent.

Replace use of styled-components/euiThemevars with @emotion/styled

f52586f

Remove usage of styled-components and EuiThemeVars from ExceptionsLis…

67e9cff

…tCard This component exists as the inner item on the Shared Exception Lists page. Styles have been verified as equivalent.

Replace use of static theme vars via @emotion/styled

114b07c

Remove use of static theme vars in ValueListModal body

528d968

Remove static theme vars from Threshold Rule Creation components

9299dda

* Defines a FieldSectionGroup component instead of applying the same styles to multiple identical EuiFieldGroup components * Replaces use of margin-top with a more appropriate `align-self` declaration

[CI] Auto-commit changed files from 'node scripts/styled_components_m…

738ebe3

…apping'

Fix styles on "Linked Rules" link in Rule Exceptions Tab

b95cf8f

These were already fixed on the package version of this component, but the one that's still in the plugin was not updated, which I noticed while taking screenshots.

Add EuiTheme to @emotion types in exception-list package

42e9dcb

Fix/update tests following new use of EuiTheme

eb60210

These tests were failing because they lacked the EUI Theme context when we were rendering these components which now require them (previously they were requiring a static object imported from elsewhere).

rylnd force-pushed the rylnd/eui_tech_debt branch from b4c12ab to eb60210 Compare January 31, 2025 23:47

rylnd added 2 commits January 31, 2025 18:00

Use clarifying testing syntax

d2faf9b

We don't really want to test the EuiThemeProvider, it's merely a consequence of the component's dependencies/how it's written. Calling the provider out as a wrapper better helps to show that relationship.

Merge branch 'main' into rylnd/eui_tech_debt

eca40cd

Merge branch 'main' into rylnd/eui_tech_debt

7684a34

rylnd marked this pull request as ready for review February 1, 2025 18:07

rylnd requested review from a team as code owners February 1, 2025 18:07

rylnd requested a review from vitaliidm February 1, 2025 18:07

jbudz approved these changes Feb 3, 2025

View reviewed changes

vitaliidm reviewed Feb 3, 2025

View reviewed changes

Merge branch 'main' into rylnd/eui_tech_debt

3df4d2d

vitaliidm approved these changes Feb 4, 2025

View reviewed changes

rylnd merged commit 39ec0a0 into elastic:main Feb 5, 2025
9 checks passed

rylnd deleted the rylnd/eui_tech_debt branch February 5, 2025 01:58

kibanamachine added the v9.1.0 label Feb 5, 2025

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Security Solution][Detection Engine] EUI Tech Debt - Removes references to static EuiTheme variables#208820

[Security Solution][Detection Engine] EUI Tech Debt - Removes references to static EuiTheme variables#208820
rylnd merged 25 commits intoelastic:mainfrom
rylnd:rylnd/eui_tech_debt

rylnd commented Jan 29, 2025 •

edited

Loading

Uh oh!

rylnd commented Jan 29, 2025

Uh oh!

rylnd commented Jan 29, 2025

Uh oh!

rylnd commented Jan 31, 2025

Uh oh!

rylnd commented Feb 1, 2025

Uh oh!

elasticmachine commented Feb 1, 2025

Uh oh!

vitaliidm Feb 3, 2025 •

edited

Loading

Uh oh!

rylnd Feb 4, 2025

Uh oh!

vitaliidm Feb 4, 2025

Uh oh!

elasticmachine commented Feb 4, 2025

Uh oh!

vitaliidm left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

rylnd commented Jan 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Screenshots of Areas Affected

Rule Exceptions Tab

Before

After

Shared Exception Lists

Shared Exception List Details

Threshold Input

Checklist

Uh oh!

rylnd commented Jan 29, 2025

Uh oh!

rylnd commented Jan 29, 2025

Uh oh!

rylnd commented Jan 31, 2025

Uh oh!

rylnd commented Feb 1, 2025

Uh oh!

elasticmachine commented Feb 1, 2025

Uh oh!

vitaliidm Feb 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rylnd Feb 4, 2025

Choose a reason for hiding this comment

Uh oh!

vitaliidm Feb 4, 2025

Choose a reason for hiding this comment

Uh oh!

elasticmachine commented Feb 4, 2025

💚 Build Succeeded

Metrics [docs]

Module Count

Async chunks

History

Uh oh!

vitaliidm left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

rylnd commented Jan 29, 2025 •

edited

Loading

vitaliidm Feb 3, 2025 •

edited

Loading