-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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] Migrate infra from styled-components to @emotion #202405
[Infra] Migrate infra from styled-components to @emotion #202405
Conversation
Pinging @elastic/obs-ux-infra_services-team (Team:obs-ux-infra_services) |
🤖 GitHub commentsExpand to view the GitHub comments
Just comment with:
|
@@ -56,7 +57,7 @@ export const Threshold = ({ | |||
data-test-subj={`threshold-${thresholds.join('-')}-${value}`} | |||
> | |||
<Chart> | |||
<Settings theme={theme} baseTheme={baseTheme} locale={i18n.getLocale()} /> | |||
<Settings theme={theme as PartialTheme} baseTheme={baseTheme} locale={i18n.getLocale()} /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we need to use as PartialTheme
here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The types coming from the emotion Theme and the type requested for Settings theme doesn't match, we need to cast it as PartialTheme
to work, I'm open to suggestions on how to fix it in a different way
@@ -64,7 +64,7 @@ describe('Threshold', () => { | |||
|
|||
expect((Metric as jest.Mock).mock.calls[0][0].data[0][0]).toMatchInlineSnapshot(` | |||
Object { | |||
"color": "#f8e9e9", | |||
"color": "#BD271E", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Won't this potentially be an issue for flakiness should there be any further changes to the design system or theming?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought so, I will check if we can actually not pass the color for the test, or another option
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added some nits but haven't tested it yet ( I will add a separate comment if I find issues while testing but the code looks good 💯 )
x-pack/plugins/observability_solution/infra/public/pages/metrics/metric_detail/types.ts
Outdated
Show resolved
Hide resolved
...ck/plugins/observability_solution/infra/public/alerting/common/components/threshold.test.tsx
Outdated
Show resolved
Hide resolved
...lugins/observability_solution/infra/public/components/autocomplete_field/suggestion_item.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/observability_solution/infra/public/alerting/common/components/threshold.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
...bility_solution/infra/public/components/logging/log_analysis_results/category_expression.tsx
Show resolved
Hide resolved
...plugins/observability_solution/infra/public/components/logging/log_minimap/density_chart.tsx
Show resolved
Hide resolved
...k/plugins/observability_solution/infra/public/components/logging/log_minimap/log_minimap.tsx
Show resolved
Hide resolved
...k/plugins/observability_solution/infra/public/components/logging/log_minimap/log_minimap.tsx
Show resolved
Hide resolved
flex-direction: row; | ||
`; | ||
|
||
LogStatusbar.defaultProps = { alignItems: 'center', gutterSize: 'none', justifyContent: 'flexEnd' }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: this is a deprecated React feature, prefer switching over a function react component if you need default props.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Found this as a suggestion to replace the attrs on styled components but you're right it's a bit old, I'll see if I found something else
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hope this helps, it should make the trick for this case (coded only, check if everything works fine on Kibana please):
export const LogStatusbar = (props: EuiFlexGroupProps) => {
const { euiTheme } = useEuiTheme();
return (
<EuiFlexGroup
alignItems="center"
gutterSize="none"
justifyContent="flexEnd"
css={css`
padding: ${euiTheme.size.s};
border-top: ${euiTheme.border.thin};
max-height: 48px;
min-height: 48px;
background-color: ${euiTheme.colors.emptyShade};
flex-direction: row;
`}
{...props}
/>
);
};
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MiriamAparicio I left some notes but there are more similar across the PR, the color tokens used as shades (emptyShade
, 'darkShade`, etc...) are all deprecated and should change with other colors from the palette, this internal guide might help you picking the right color.
Just seen your comment here, if the EUI team is ok keeping those around, I'm 👌
@tonyghiani the comment wasn't in slack but in another PR related with the visual refresh, #200814 (comment) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I saw some errors while testing (And added some questions about some background colors):
-
Undefined values:
-
Colors:
- [Light Mode | Borealis] Host Details -> Processes tab -> Process charts: With the new background the chart lines are barely visible:
x-pack/plugins/observability_solution/infra/public/alerting/common/components/threshold.tsx
Outdated
Show resolved
Hide resolved
.../observability_solution/infra/public/alerting/metric_threshold/components/expression_row.tsx
Show resolved
Hide resolved
...lugins/observability_solution/infra/public/components/autocomplete_field/suggestion_item.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/observability_solution/infra/public/components/logging/log_statusbar.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/observability_solution/infra/public/components/logs_deprecation_callout.tsx
Show resolved
Hide resolved
background: ${(props) => props.theme.eui.euiColorGhost}; | ||
width: calc(100% + ${(props) => props.theme.eui.euiSizeL} * 2); | ||
margin-left: -${(props) => props.theme.eui.euiSizeL}; | ||
background: ${(props) => props.theme.euiTheme.colors.ghost}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: Is it expected that this container always has a white background, also in dark mode?
If it should update the background based on colorMode
(having a dark background in dark mode) then this should rather use backgroundBasePlain
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will change it as ghost is deprecated already
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
...vability_solution/infra/public/pages/metrics/inventory_view/components/waffle/group_name.tsx
Outdated
Show resolved
Hide resolved
.../public/pages/metrics/inventory_view/components/waffle/metric_control/custom_metric_form.tsx
Outdated
Show resolved
Hide resolved
...olution/infra/public/pages/metrics/inventory_view/components/waffle/waffle_sort_controls.tsx
Outdated
Show resolved
Hide resolved
...ervability_solution/infra/public/pages/metrics/metric_detail/components/metadata_details.tsx
Outdated
Show resolved
Hide resolved
23e74d1
to
672caf8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tested again and it looks good now 💯
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the updates! Awesome job migrating off styled components! 🎉
Changes LGTM from EUI side 👍
c49a950
to
12d44a4
Compare
Hi @miriam Using https://tkajtoch-pr-202405-202255-poc-migration-to-emotion.kbndev.co/
But I was able to check Metrics Explorer & Hosts:Visualizations background won't work correctly, but I think updating them is not included in this iteration. Is that correct? Hosts:In the condition builder syntax, when creating an alert, the text (FOR, WHEN) should use Also, pending confirmation from EUI from misusage on the |
Yeah, I thought it will lack some data, there was a deployment made by @kuisathaverat but I think it's not available anymore
Correct, that should be fixed by the Charts team
I'll take a look |
@patpscal
The component from eui Please @mgadewoll correct me if I'm wrong |
⏳ Build in-progress
History
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No idea why this file ended up in my PR after the merge, can you check if it's ok and approve? @elastic/search-kibana
import type { PartialTheme, Theme } from '@elastic/charts'; | ||
import { i18n } from '@kbn/i18n'; | ||
import { COMPARATORS } from '@kbn/alerting-comparators'; | ||
|
||
export interface ChartProps { | ||
theme?: PartialTheme; | ||
theme?: UseEuiTheme<{}>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would this work? {}
is the default value of the generic parameter
theme?: UseEuiTheme<{}>; | |
theme?: UseEuiTheme; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think so, i'll fix it on the next PR
Hi @MiriamAparicio, got it. I just learned the EUI team will review their definition here https://github.com/orgs/elastic/projects/1150/views/12?pane=issue&itemId=90262619 |
Apologies for getting into the conversation late, but I saw that you were describing an issue with a chart color that used to be blue and now with Borealis appears light teal. How to deal with these cases is explained in this document, within the Datavis section. To make it simple, the issue is that the order of colors within the datavis palette is now changed and Blue is now in a different place than before. Therefore, you just need to use I hope this could help clarifying the issue, please feel free to ping me in case of need! |
Hi @gvnmagni |
Friendly reminder: Looks like this PR hasn’t been backported yet. |
Closes elastic#202255 ### Summary While working on the visual refresh for the new EUI theme Borealis we figured that was a good time to do the recommended migration from styled-components to @emotion ### What has been done - Migrate infra plugin from styled-components to @emotion - Eui Visual Refresh for Borealis new theme - All usage of color palette tokens and functions now pull from the theme, and correctly update to use new colors when the theme changes from Borealis to Amsterdam and vice versa - All references to renamed tokens have been updated to use the new token name - Remove usage of deprecated `useEuiBackgroundColor` - All usages of "success" colors have been updated to `accentSecondary` and `textAccentSecondary` as needed ### How to test #### Running Kibana with the Borealis theme In order to run Kibana with Borealis, you'll need to do the following: - Set the following in kibana.dev.yml: `uiSettings.experimental.themeSwitcherEnabled: true` - Run Kibana with the following environment variable set: `KBN_OPTIMIZER_THEMES="borealislight,borealisdark,v8light,v8dark" yarn start` - This will expose a toggle under Stack Management > Advanced Settings > Theme version, which you can use to toggle between Amsterdam and Borealis.
) ## Summary After migrating infra plugin from `styled-components` to `@emotion` in this [PR](#202405), this file was missed to update
Closes #202255
Summary
While working on the visual refresh for the new EUI theme Borealis we figured that was a good time to do the recommended migration from styled-components to @emotion
What has been done
useEuiBackgroundColor
accentSecondary
andtextAccentSecondary
as neededHow to test
Running Kibana with the Borealis theme
In order to run Kibana with Borealis, you'll need to do the following:
uiSettings.experimental.themeSwitcherEnabled: true
KBN_OPTIMIZER_THEMES="borealislight,borealisdark,v8light,v8dark" yarn start