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] - replace usages of EUI vis colors #205218

Merged
merged 2 commits into from
Jan 10, 2025

Conversation

PhilippeOberti
Copy link
Contributor

@PhilippeOberti PhilippeOberti commented Dec 27, 2024

Summary

This PR is part of a list of PRs to perform the changes necessary to get the new Borealis theme working correctly. It focuses on replace the vis colors for non-visualization usages by correct EUI color tokens.

2 places have been impacted:

  • the alerts data table in grouping mode (for host, user and network). The background colors are removed as they don't really mean anything, and are not used anywhere else in Kibana. Also the icons have been updated to match the ones used in other places (like in the flyout)

Host

before after
data-table-host-main data-table-host-new

User

before after
data-table-user-main data-table-user-new

Network

before after
data-table-network-main data-table-network-new
  • the dataprovider badge in timeline

Dark mode

before after
timeline-main-dark timeline-new-dark

Light mode

before after
timeline-main-light timeline-new-light

The changes have been reviewed and approved by @codearos 😄

#201883

@PhilippeOberti PhilippeOberti added release_note:skip Skip the PR/issue when compiling release notes backport:skip This commit does not require backporting v9.0.0 Team:Threat Hunting:Investigations Security Solution Investigations Team EUI Visual Refresh labels Dec 27, 2024
@PhilippeOberti PhilippeOberti requested review from a team as code owners December 27, 2024 19:00
@elasticmachine
Copy link
Contributor

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

Copy link
Contributor

@angorayc angorayc left a comment

Choose a reason for hiding this comment

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

Found that we no longer have background color of the group icons after migration, is it intended?

@PhilippeOberti
Copy link
Contributor Author

PhilippeOberti commented Jan 10, 2025

Found that we no longer have background color of the group icons after migration, is it intended?

@angorayc yeah this is intentional. The background colors behind those icons made no sense to me. I'm not sure why they were added in the first place. Why would we have colors behind host, user and network icons but not behind any other icons? Also, we're not reusing these colors anywhere else in Kibana (that I'm aware of) for host, user and network. We're even using different icons (which this PR also fixes).

I just reached out to @codearos on Slack to make sure he is ok with this change. I will update the PR description to add this information once confirmed it's validated!

@PhilippeOberti
Copy link
Contributor Author

PhilippeOberti commented Jan 10, 2025

Found that we no longer have background color of the group icons after migration, is it intended?

@angorayc yeah this is intentional. The background colors behind those icons made no sense to me. I'm not sure why they were added in the first place. Why would we have colors behind host, user and network icons but not behind any other icons? Also, we're not reusing these colors anywhere else in Kibana (that I'm aware of) for host, user and network. We're even using different icons (which this PR also fixes).

I just reached out to @codearos on Slack to make sure he is ok with this change. I will update the PR description to add this information once confirmed it's validated!

no background color changes approved by @codearos!
I did remove the icon I added for the rule group per his request.
PR description updated accordingly.

Copy link
Contributor

@mgadewoll mgadewoll left a comment

Choose a reason for hiding this comment

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

✅ Changes LGTM from EUI side.

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

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 22.2MB 22.2MB -1.5KB

History

@PhilippeOberti PhilippeOberti merged commit cebd201 into elastic:main Jan 10, 2025
8 checks passed
@PhilippeOberti PhilippeOberti deleted the eui-color-vis branch January 10, 2025 22:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting EUI Visual Refresh release_note:skip Skip the PR/issue when compiling release notes Team:Threat Hunting:Investigations Security Solution Investigations Team v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants