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][THI] remove usages of EUI json tokens #210482

Merged
merged 3 commits into from
Feb 11, 2025

Conversation

PhilippeOberti
Copy link
Contributor

@PhilippeOberti PhilippeOberti commented Feb 11, 2025

Summary

This PR is probably the final PR that makes the changes to support EUI Borealis. It focuses on removing all the usage of EUI Json tokens.

You will notice different approaches while removing the tokens:

  • for some cases, the changes were done using css from '@emotions/react' as the components using the tokens were already using euiTheme or adding it was straightforward and required the minimal amount of changes
  • for some cases, where the css changes were pretty involved, a hook was created to be able to import the styles and apply them in the components
  • finally for other cases, especially if the styled components were extracted in a different file and were used within many others, I decided to create reusable components. This allowed to not change all the files impacted and limit the number of files modified in this PR.

Feel free to comment on any of the approaches and suggest better options!

#201889

@PhilippeOberti PhilippeOberti added release_note:skip Skip the PR/issue when compiling release notes Team:Threat Hunting:Investigations Security Solution Investigations Team backport:version Backport to applied version labels v9.1.0 v8.19.0 labels Feb 11, 2025
@PhilippeOberti PhilippeOberti requested a review from a team as a code owner February 11, 2025 00:52
@elasticmachine
Copy link
Contributor

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

@@ -178,7 +177,7 @@ describe('helpers', () => {
});

describe('addBuildingBlockStyle', () => {
const THEME = { eui: euiThemeVars, darkMode: false };
const THEME = { eui: { euiColorHighlight: 'euiColorHighlight' }, darkMode: false } as EuiTheme;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

while type casting isn't ideal here, I couldn't find any mocks that could be used...

@@ -46,7 +44,7 @@ export const getActionsColumnWidth = (actionButtonCount: number): number => {
// `EuiDataGridRowCell` applies additional `padding-left` and
// `padding-right`, which must be added to the content width to prevent the
// content from being partially hidden due to the space occupied by padding:
const leftRightCellPadding = parseInt(euiThemeVars.euiDataGridCellPaddingM, 10) * 2; // parseInt ignores the trailing `px`, e.g. `6px`
const leftRightCellPadding = 12;
Copy link
Contributor Author

@PhilippeOberti PhilippeOberti Feb 11, 2025

Choose a reason for hiding this comment

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

this change isn't ideal either, but:>

  • EUI is not currently a replacement for euiDataGridCellPaddingM (unless I'm missing something)
  • due to the way the getActionsColumnWidth function is used, I'm not even sure how we would pass data to it?

Also I checked in EUI repo, and the value for euiDataGridCellPaddingM hasn't changed since 2021... So it felt safe-ish to hardcode the value here...

@elastic-vault-github-plugin-prod elastic-vault-github-plugin-prod bot requested a review from a team as a code owner February 11, 2025 01:02
@PhilippeOberti PhilippeOberti enabled auto-merge (squash) February 11, 2025 19:15
@PhilippeOberti PhilippeOberti merged commit 161ce34 into elastic:main Feb 11, 2025
9 checks passed
@kibanamachine
Copy link
Contributor

Starting backport for target branches: 8.x

https://github.com/elastic/kibana/actions/runs/13272591177

@elasticmachine
Copy link
Contributor

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] FTR Configs #85 / discover/group4 adhoc data views search results should be different after data view update
  • [job] [logs] FTR Configs #5 / X-Pack Accessibility Tests - Group 1 uptime Accessibility detail page

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 24.9MB 24.9MB -1.4KB
Unknown metric groups

References to deprecated APIs

id before after diff
securitySolution 357 352 -5

Unreferenced deprecated APIs

id before after diff
@kbn/ui-theme 2 3 +1

History

@PhilippeOberti PhilippeOberti deleted the json-tokens branch February 11, 2025 21:13
kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Feb 11, 2025
…0482)

## Summary

This PR is probably the final PR that makes the changes to support EUI
Borealis. It focuses on removing all the usage of EUI Json tokens.

You will notice different approaches while removing the tokens:
- for some cases, the changes were done using `css from
'@emotions/react'` as the components using the tokens were already using
`euiTheme` or adding it was straightforward and required the minimal
amount of changes
- for some cases, where the css changes were pretty involved, a hook was
created to be able to import the styles and apply them in the components
- finally for other cases, esepcially if the styled components were
extracted in a different file and were used within many others, I
decided to create reusable components. This allowed to not change all
the files impacted and limit the number of files modified in this PR.

Feel free to comment on any of the approaches and suggest better
options!

elastic#201889

---------

Co-authored-by: kibanamachine <[email protected]>
(cherry picked from commit 161ce34)
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
8.x

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 Feb 11, 2025
) (#210700)

# Backport

This will backport the following commits from `main` to `8.x`:
- [[Security Solution][THI] remove usages of EUI json tokens
(#210482)](#210482)

<!--- Backport version: 9.4.3 -->

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

<!--BACKPORT [{"author":{"name":"Philippe
Oberti","email":"[email protected]"},"sourceCommit":{"committedDate":"2025-02-11T21:11:55Z","message":"[Security
Solution][THI] remove usages of EUI json tokens (#210482)\n\n##
Summary\r\n\r\nThis PR is probably the final PR that makes the changes
to support EUI\r\nBorealis. It focuses on removing all the usage of EUI
Json tokens.\r\n\r\nYou will notice different approaches while removing
the tokens:\r\n- for some cases, the changes were done using `css
from\r\n'@emotions/react'` as the components using the tokens were
already using\r\n`euiTheme` or adding it was straightforward and
required the minimal\r\namount of changes\r\n- for some cases, where the
css changes were pretty involved, a hook was\r\ncreated to be able to
import the styles and apply them in the components\r\n- finally for
other cases, esepcially if the styled components were\r\nextracted in a
different file and were used within many others, I\r\ndecided to create
reusable components. This allowed to not change all\r\nthe files
impacted and limit the number of files modified in this PR.\r\n\r\nFeel
free to comment on any of the approaches and suggest
better\r\noptions!\r\n\r\nhttps://github.com//issues/201889\r\n\r\n---------\r\n\r\nCo-authored-by:
kibanamachine
<[email protected]>","sha":"161ce34cf7f38faed884421976b10494df0a4075","branchLabelMapping":{"^v9.1.0$":"main","^v8.19.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","Team:Threat
Hunting:Investigations","backport:version","v9.1.0","v8.19.0"],"title":"[Security
Solution][THI] remove usages of EUI json
tokens","number":210482,"url":"https://github.com/elastic/kibana/pull/210482","mergeCommit":{"message":"[Security
Solution][THI] remove usages of EUI json tokens (#210482)\n\n##
Summary\r\n\r\nThis PR is probably the final PR that makes the changes
to support EUI\r\nBorealis. It focuses on removing all the usage of EUI
Json tokens.\r\n\r\nYou will notice different approaches while removing
the tokens:\r\n- for some cases, the changes were done using `css
from\r\n'@emotions/react'` as the components using the tokens were
already using\r\n`euiTheme` or adding it was straightforward and
required the minimal\r\namount of changes\r\n- for some cases, where the
css changes were pretty involved, a hook was\r\ncreated to be able to
import the styles and apply them in the components\r\n- finally for
other cases, esepcially if the styled components were\r\nextracted in a
different file and were used within many others, I\r\ndecided to create
reusable components. This allowed to not change all\r\nthe files
impacted and limit the number of files modified in this PR.\r\n\r\nFeel
free to comment on any of the approaches and suggest
better\r\noptions!\r\n\r\nhttps://github.com//issues/201889\r\n\r\n---------\r\n\r\nCo-authored-by:
kibanamachine
<[email protected]>","sha":"161ce34cf7f38faed884421976b10494df0a4075"}},"sourceBranch":"main","suggestedTargetBranches":["8.x"],"targetPullRequestStates":[{"branch":"main","label":"v9.1.0","branchLabelMappingKey":"^v9.1.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/210482","number":210482,"mergeCommit":{"message":"[Security
Solution][THI] remove usages of EUI json tokens (#210482)\n\n##
Summary\r\n\r\nThis PR is probably the final PR that makes the changes
to support EUI\r\nBorealis. It focuses on removing all the usage of EUI
Json tokens.\r\n\r\nYou will notice different approaches while removing
the tokens:\r\n- for some cases, the changes were done using `css
from\r\n'@emotions/react'` as the components using the tokens were
already using\r\n`euiTheme` or adding it was straightforward and
required the minimal\r\namount of changes\r\n- for some cases, where the
css changes were pretty involved, a hook was\r\ncreated to be able to
import the styles and apply them in the components\r\n- finally for
other cases, esepcially if the styled components were\r\nextracted in a
different file and were used within many others, I\r\ndecided to create
reusable components. This allowed to not change all\r\nthe files
impacted and limit the number of files modified in this PR.\r\n\r\nFeel
free to comment on any of the approaches and suggest
better\r\noptions!\r\n\r\nhttps://github.com//issues/201889\r\n\r\n---------\r\n\r\nCo-authored-by:
kibanamachine
<[email protected]>","sha":"161ce34cf7f38faed884421976b10494df0a4075"}},{"branch":"8.x","label":"v8.19.0","branchLabelMappingKey":"^v8.19.0$","isSourceBranch":false,"state":"NOT_CREATED"}]}]
BACKPORT-->

Co-authored-by: Philippe Oberti <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:version Backport to applied version labels release_note:skip Skip the PR/issue when compiling release notes Team:Threat Hunting:Investigations Security Solution Investigations Team v8.19.0 v9.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants