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 tint, shade and transparentize functions #205223

Merged
merged 8 commits into from
Jan 14, 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 removing the shade(), tint(), shadeOrTint(), tintOrShade() and transparentize() functions.

2 places have been impacted:

  • the expandable flyout preview shadow

Light

before after
flyout-preview-main-light flyout-preview-new-light

Dark

before after
flyout-preview-main-dark flyout-preview-new-dark
  • the unified data table
    • for row hover background color (I tried a few options here, this change is the one that looked the best to me, despite being identical color to the odd row background... I'm opened to suggestions here!)

Light

before after
table-hover-main-light table-hover-new-light

Dark

before after
table-hover-main-dark table-hover-new-dark
  • for row comparison

Light

before after
table-compare-main-light table-compare-new-light

Dark

before after
table-compare-main-dark table-compare-new-dark

Notes: one usage of the transparentize function was left untouched here. This is a custom OverlayMask and this code replicates what's being done in the EUI codebase. I checked and EUI is still using the transparentize method with that color, so I figured I would keep this untouched for now...

#201888

@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 21:29
@elasticmachine
Copy link
Contributor

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

@kertal kertal requested a review from l-suarez December 30, 2024 12:06
@kertal
Copy link
Member

kertal commented Dec 30, 2024

the unified data table
for row hover background color (I tried a few options here, this one for the one that looked the best, despite being identical color to the odd row background... I'm opened to suggestions here!)

adding @l-suarez to have a look at this, thx 🙏

Copy link
Contributor

@davismcphee davismcphee left a comment

Choose a reason for hiding this comment

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

I checked this out quickly today and agree we should get some design input for the table row hover styles. Using the same colour for both row striping and hovering makes the hovered row blend in more and less easy to identify in wider tables, but I'm not sure of an alternative without using tintOrShade. I'll ask the EUI team to take a look in Slack too in case they have ideas.

For reference, I tried out $euiColorLightShade for hovering too, but it seemed pretty harsh compared to before IMO.

Using tintOrShade (current):
before

Using $euiColorLightShade:
after

@MichaelMarcialis
Copy link
Contributor

MichaelMarcialis commented Jan 6, 2025

I checked this out quickly today and agree we should get some design input for the table row hover styles. Using the same colour for both row striping and hovering makes the hovered row blend in more and less easy to identify in wider tables, but I'm not sure of an alternative without using tintOrShade. I'll ask the EUI team to take a look in Slack too in case they have ideas.

@davismcphee: In case it helps, I think data grid row hover would call for using euiTheme.colors.backgroundBaseInteractiveHover. However, I would assume this style would be packaged with the EUI component, rather than something that requires separate styles.

@davismcphee davismcphee force-pushed the eui-functions-cleanup branch from 1e818e7 to fb383d6 Compare January 7, 2025 03:20
@davismcphee
Copy link
Contributor

@PhilippeOberti I hope you don't mind, I merged main and pushed a commit to adjust Unified Data Table to use $euiColorBackgroundBaseInteractiveHover.

@MichaelMarcialis Thanks for the hint. I was chatting with EUI folks earlier and they pointed me in a similar direction too, so I gave it a shot. For context, the issue was that for Amsterdam we have an override since the default data grid styles use a yellow highlight that we didn't want (I'm guessing since it conflicts with our "expanded" row styles). The hover styles have been changed in Borealis, but I believe we're supposed to maintain support for both Amsterdam and Borealis in main for now, so it was tricky to find an override that works for both.

I pushed a change to use $euiColorBackgroundBaseInteractiveHover for both Amsterdam and Borealis (already the default there), which still changes the colour in Amsterdam a bit, but it looks better than $euiColorLightShade at least. If this seems ok from the design side, I'm good to move forward with it and eventually remove the override entirely once Amsterdam is retired. WDYT?
image

Copy link
Contributor

@davismcphee davismcphee left a comment

Choose a reason for hiding this comment

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

Looks like CI is green now so I'm gonna approve from the Data Discovery side since I'm good with the current row hover override until we can remove it when Amsterdam is retired 👍

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

@PhilippeOberti PhilippeOberti enabled auto-merge (squash) January 14, 2025 00:00
@elasticmachine
Copy link
Contributor

💔 Build Failed

Failed CI Steps

History

@PhilippeOberti PhilippeOberti merged commit 3057eac into elastic:main Jan 14, 2025
8 checks passed
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.

7 participants