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] Test and fix EUI theme Borealis applied to Rule Management features #204737

Closed
5 tasks done
banderror opened this issue Dec 18, 2024 · 10 comments · Fixed by #206122
Closed
5 tasks done
Assignees
Labels
9.0 candidate bug Fixes for quality problems that affect the customer experience EUI Visual Refresh Feature:Rule Details Security Solution Detection Rule Details page Feature:Rule Management Security Solution Detection Rule Management area impact:low Addressing this issue will have a low level of impact on the quality/strength of our product. Team:Detection Rule Management Security Detection Rule Management Team Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc.

Comments

@banderror
Copy link
Contributor

banderror commented Dec 18, 2024

Related to: #199715
PR: #206122

Summary

First initial testing done by @banderror revealed no major issues with the features owned by the @elastic/security-detection-rule-management team. In fact, the new theme looks very nice! A few very minor issues were noticed that are logged below.

We should regression test our UIs and follow #199715 for the work that needs to be done in the code.

Todo

  • Check if Borealis has been enabled by default. #temp-security-eui-borealis-integration
  • Review the actions requested by the EUI team and understand what do we need to do exactly
  • Test our features with the theme enabled and update the "Noticed issues" section below

Noticed issues

Colors

Some colors that we use for rule status and severity indicators seem off.

Current theme Borealis
Image Image
Image Image
Image Image
(the warning color appears way too dark)
@banderror banderror added 9.0 candidate bug Fixes for quality problems that affect the customer experience Feature:Rule Details Security Solution Detection Rule Details page Feature:Rule Management Security Solution Detection Rule Management area impact:low Addressing this issue will have a low level of impact on the quality/strength of our product. Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc. Team:Detection Rule Management Security Detection Rule Management Team Team:Detections and Resp Security Detection Response Team labels Dec 18, 2024
@elasticmachine
Copy link
Contributor

Pinging @elastic/security-solution (Team: SecuritySolution)

@elasticmachine
Copy link
Contributor

Pinging @elastic/security-detections-response (Team:Detections and Resp)

@elasticmachine
Copy link
Contributor

Pinging @elastic/security-detection-rule-management (Team:Detection Rule Management)

@banderror banderror changed the title [Security Solution] Test and fix EUI theme Borealis applied to Rule Management features (DRAFT) [Security Solution] Test and fix EUI theme Borealis applied to Rule Management features Dec 18, 2024
@JasonStoltz
Copy link
Member

@banderror Interesting issue you're experiencing!

My hunch is that you are using euiColorVis colors for those. As part of the advice for Borealis, we are asking folks to discontinue using those outside of data visualizations.

My recommendation here is to stick with the EuiHealth component here (if you're not using it already) and using the colors as described in the example:

<EuiHealth color="subdued">Inactive</EuiHealth>
<EuiHealth color="primary">Active</EuiHealth>
<EuiHealth color="success">Healthy</EuiHealth>
<EuiHealth color="warning">Warning</EuiHealth>
<EuiHealth color="danger">Failure</EuiHealth>

CC @gvnmagni and @mgadewoll to see if they agree!

@gvnmagni
Copy link

thank you Jason and thank you @banderror . I am aware of this case, @codearos shared this to me few days ago since I was trying to map different uses of severity levels across the product.

We have been working on a Severity Color Palette that might help you out here, it is still a work in progress but you can already find a description here (Issue & conversation) and a PR made by Lene here that introduces tokens (PR).

If you like the idea, and I would love to hear from @codearos here first maybe, we could try use colors coming from this palette.

If, instead, you prefer a quick fix, we can try to match similar color to the ones you had in Amsterdam and make the best out of this (the reason why colors changed is bacause Datavis colors palette have been updated and they don't follow the same logic as before, this is why we have undesired results now)

@nikitaindik
Copy link
Contributor

nikitaindik commented Jan 13, 2025

I've thoroughly tested our pages and opened a PR that implements the changes described in "Requested Code Changes" of this issue.

Incorrect severity colors will be fixed by this Threat Hunting PR.

Incorrect color of the warning rule execution status will be addressed by EUI. There is no draft PR yet, as discussions are still ongoing between the design and EUI folks to determine the best approach.

We can consider this issue done once my PR and the severity colors PR are merged.

@banderror
Copy link
Contributor Author

Hey @nikitaindik, let's keep the ticket open for now. As you say, when both PRs are merged, we will need to do some final testing and close it if everything is ok.

@banderror banderror reopened this Jan 14, 2025
@banderror banderror marked this as a duplicate of #207021 Jan 17, 2025
viduni94 pushed a commit to viduni94/kibana that referenced this issue Jan 23, 2025
…lastic#206122)

**Resolves: elastic#204737**
**Related PR (fixes severity indicator colors): elastic#206276**

## Summary
This PR introduces some of the necessary changes to support the Borealis
UI theme on Rule Management team-owned pages. You can find a list of
what needs to be done in this
[issue](elastic#199715) description
under "Requested Code Changes".

## Changes in this PR
- Replaced deprecated `euiThemeVars` with `useEuiTheme`.
- Fixed incorrect "disabled" button color for the rule delete bulk
action.

## Changes that are needed, but are outside of this PR's scope
- Fix incorrect severity colors in Borealis. Will be resolved by this
Threat Hunting [PR](elastic#206276).
<img width="93" alt="Scherm­afbeelding 2025-01-13 om 13 46 26"
src="https://github.com/user-attachments/assets/d35407d7-77f9-4d7d-a6b8-3dfe4f8a0e9e"
/>

- Fix rule execution status indicator appearing too dark in the Borealis
theme. This issue will be addressed in EUI, but there is no draft PR
yet, as discussions are still ongoing between the design team and EUI
folks to determine the best approach.
<img width="326" alt="borealis"
src="https://github.com/user-attachments/assets/3324448c-be13-4074-bfdc-c6837fb2bc6c"
/>

## Testing
You can find the theme switcher in "Stack Management" -> "Advanced
Settings". Then you can test if everything looks correct in:
- Amsterdam theme, light mode
- Amsterdam theme, dark mode
- Borealis theme, light mode
- Borealis theme, dark mode

Work started on: 08-01-2025
@nikitaindik
Copy link
Contributor

The severity colors PR is now finally merged. I'll test our merged changes combined with new severity colors on main and will post an update here.

@nikitaindik
Copy link
Contributor

nikitaindik commented Jan 27, 2025

I retested our pages in both themes with dark and light modes, and can confirm that the changes look good and we are now Borealis-ready.

Here's a screenshot of severity colors on the Rule Management page for Amsterdam and Borealis.
Image

Since the severity colors palette is still in development by the design folks (issue), the colors may change in the future. However, severity colors for the whole Security Solution are now centralized in one place (code), making future updates straightforward once the final palette is ready.

Let me get a final confirmation from the designer that we are good to go and ask him to check the "QA" checkbox for our team in the ticket. Apart from that, there's really nothing left to do from our end.

@nikitaindik
Copy link
Contributor

We have a ✅ for QA from the designer. Closing this one! 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
9.0 candidate bug Fixes for quality problems that affect the customer experience EUI Visual Refresh Feature:Rule Details Security Solution Detection Rule Details page Feature:Rule Management Security Solution Detection Rule Management area impact:low Addressing this issue will have a low level of impact on the quality/strength of our product. Team:Detection Rule Management Security Detection Rule Management Team Team:Detections and Resp Security Detection Response Team Team: SecuritySolution Security Solutions Team working on SIEM, Endpoint, Timeline, Resolver, etc.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants