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

[Infra Alerting UI] Audit new EUI Borealis theme #205142

Merged
merged 9 commits into from
Jan 13, 2025

Conversation

fkanout
Copy link
Contributor

@fkanout fkanout commented Dec 24, 2024

Summary

It fixes #205052 by

  • Remapping the vis color for the Infra rule preview chart. This is a temporary solution because the current is not visible, so we are waiting for @maciejforcone inputs.
  • Deleting the unused Expression chart

@fkanout fkanout requested review from a team as code owners December 24, 2024 13:00
@fkanout fkanout self-assigned this Dec 24, 2024
@fkanout fkanout added Feature:Alerting Team:obs-ux-management Observability Management User Experience Team EUI Visual Refresh labels Dec 24, 2024
@elasticmachine
Copy link
Contributor

Pinging @elastic/obs-ux-management-team (Team:obs-ux-management)

@fkanout fkanout added release_note:skip Skip the PR/issue when compiling release notes backport:skip This commit does not require backporting labels Dec 24, 2024
@fkanout fkanout requested review from kkurstak and patpscal December 24, 2024 13:11
Copy link
Contributor

@MiriamAparicio MiriamAparicio left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -28,16 +28,19 @@ export type Palette = {
const euiPalette = euiPaletteColorBlind();
Copy link
Contributor

@mgadewoll mgadewoll Jan 7, 2025

Choose a reason for hiding this comment

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

euiPaletteColorBlind used to be static in Amsterdam as we only had a single set of visualization colors but it was changed to ensure support for Borealis and Amsterdam.

The default palette is currently Amsterdam (until EUI switches default theme) and on EuiProvider theme update (this is within React) this palette updates.
I don't know how this is actually used, but the code here looks static, so it would likely use the initial value (Amsterdam).
If you're finding that you have unwanted static values (meaning Amsterdam colors in Borealis) the suggested usage for this is to either declare the variable inside a component that uses it if you're in React land (to ensure it can be recalled on rerenders based on EuiProvider) or you can manually listen to the store updates that the palettes are feeding from.

// simplified code 
import { EUI_VIS_COLOR_STORE } from '@elastic/eui;

// colors from default EUI theme
let visColors = euiPaletteColorBlind();

// subscribe to the store for changes (changes happen based on EuiProvider updates)
const storeId = EUI_VIS_COLOR_STORE.subscribe(
  VIS_COLOR_STORE_EVENTS.UPDATE,
  (updatedColors) => {
    // update your colors here
    visColors = updatedColors; // or call euiPaletteColorBlind() again
  }
);

// unsubscribe
EUI_VIS_COLOR_STORE.unsubscribe(VIS_COLOR_STORE_EVENTS.UPDATE, storeId);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mgadewoll I tested the chart that uses this mapping/helpers (which is in the shared folder where the backend and frontend could use it --No React).

Screenshot 2025-01-13 at 15 18 07 Screenshot 2025-01-13 at 15 17 41 Screenshot 2025-01-13 at 15 16 51

And the colors followed the theme selection. Does that cover your concerns?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, thanks for checking an clarifying! 🙏
As long as the expected colors are propagated for all use cases, it's fine to keep as is!

@patpscal
Copy link

patpscal commented Jan 7, 2025

Hi @fkanout! For some reason, trying to run yarn es snapshot returns an error if switched to this PR. Could you share some screenshots with me, or perhaps we could do a pair review? Thank you!

@fkanout
Copy link
Contributor Author

fkanout commented Jan 10, 2025

Hi @fkanout! For some reason, trying to run yarn es snapshot returns an error if switched to this PR. Could you share some screenshots with me, or perhaps we could do a pair review? Thank you!

Hi @patpscal , I was out sick, hence the delayed reply! Thanks for checking the PR. It was outdated and had some merge conflicts, which could explain the error you had. I tried yarn es snapshot, and it works as expected.

@fkanout
Copy link
Contributor Author

fkanout commented Jan 10, 2025

@patpscal if the error you are referring to is related to the ml, you need to run es with yarn es snapshot --license trial -E xpack.ml.enabled=false. But that should not be related to my PR.

@elasticmachine
Copy link
Contributor

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] Jest Tests #9 / CasesWebhookActionConnectorFields renders Validation validates correctly "viewIncidentUrlInput"
  • [job] [logs] FTR Configs #93 / visualize app visual builder Time Series Elastic charts should display correct chart data, label names and area colors for sum aggregation when split by terms

Metrics [docs]

✅ unchanged

History

cc @fkanout

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.

Copy link

@patpscal patpscal left a comment

Choose a reason for hiding this comment

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

Looks good to me

@fkanout fkanout merged commit 751de26 into elastic:main Jan 13, 2025
8 checks passed
delanni pushed a commit to delanni/kibana that referenced this pull request Jan 13, 2025
## Summary
It fixes elastic#205052 by 
- Remapping the vis color for the Infra rule preview chart. This is a
temporary solution because the current is not visible, so we are waiting
for @maciejforcone inputs.
- Deleting the unused Expression chart
viduni94 pushed a commit to viduni94/kibana that referenced this pull request Jan 23, 2025
## Summary
It fixes elastic#205052 by 
- Remapping the vis color for the Infra rule preview chart. This is a
temporary solution because the current is not visible, so we are waiting
for @maciejforcone inputs.
- Deleting the unused Expression chart
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 Feature:Alerting release_note:skip Skip the PR/issue when compiling release notes Team:obs-ux-management Observability Management User Experience Team v9.0.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Infra Alerting UI] Audit new EUI Borealis theme
6 participants