[ILM] Remove euiThemeVars and remapping colors for Borealis #204449
Conversation
SoniaSanzV
added
Feature:ILM
Team:Kibana Management
|
Pinging @elastic/kibana-management (Team:Kibana Management) |
SoniaSanzV
changed the title
|
Ran locally, looks good BUT I'd prefer to get some expert design code eyes on this so I'm withholding my approval for now. |
There was a problem hiding this comment.
LGTM 💚 I compared the values against the table, they're correct. If we need to use different vis colors for Borealis, that is the correct way to do it 👍🏻
I'll refer to @mgadewoll just in case she has more thoughts on the changes.
...cle_management/public/application/sections/edit_policy/components/phase_icon/phase_icon.scss
Outdated
Show resolved
Hide resolved
| &__inner--hot { | ||
| fill: $euiColorVis9_behindText; | ||
| &--borealis { |
There was a problem hiding this comment.
It would be nice to implement these changes with emotion instead of adding to the scss. Here's an example of how I did this for upgrade assistant - https://github.com/elastic/kibana/pull/205345/files
| @@ -5,11 +5,21 @@ | |||
| * 2.0. | |||
| */ | |||
|
|
|||
| /** @jsx jsx */ | |||
| // Needed for for testing out the css prop feature. See: https://emotion.sh/docs/css-prop#jsx-pragma | |||
There was a problem hiding this comment.
After adding emotion css, the affected snapshots displayed messages like this one:
<div
- class="ilmTimeline__colorBar ilmTimeline__frozenPhase__colorBar"
+ class="ilmTimeline__colorBar"
+ css="You have tried to stringify object returned from `css` function. It isn't supposed to be used directly (e.g. as value of the `className` prop), but rather handed to emotion so it can handle it (e.g. as value of `css` prop)."
/>
This was only happening in test, not in the final html. Following emotion docs and adding ** @jsx jsx * at the top of the file, fixed it
There was a problem hiding this comment.
If you have to set the JSX pragma this could indicate that you might be missing a required Emotion babel preset for using the css prop for your plugin? 🤔 (emotion docs)
💚 Build Succeeded
Metrics [docs]Async chunks
Page load bundle
History
cc @SoniaSanzV |
| const isBorealis = euiTheme.themeName === 'EUI_THEME_BOREALIS'; | ||
|
|
||
| const phaseIconColors = { | ||
| hot: isBorealis ? euiTheme.colors.vis.euiColorVis6 : euiThemeVars.euiColorVis9_behindText, |
There was a problem hiding this comment.
ℹ️ We're introducing the behindText vis colors to the euiTheme (PR). This will land in Kibana with this weeks release.
As we want to remove the JSON token usages from client (react) code, it would be great if you could either update if the release is faster than this PR or do a follow-up to update 🙏
There was a problem hiding this comment.
changes look good and work well, please make a follow up issue for https://github.com/elastic/kibana/pull/204449/files#r1907008672
Part of #203664
Summary
This PR addresses the changes for Borealis theme that are related to the ILM plugin. That means get rid of
euiThemeVarsin Boreales in favor ofuseEuiThemecolors and remapping the colors for ILM status. It also eliminates the use of_behindTextvalues in Borealis. This remapping is a mid-term solution since the Vis color palette is not been using here as this is not their intended use. In the future, probably a separate palette will be created.