-
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
[ObsUX] [APM] Migrate APM from styled-components to @emotion #204222
[ObsUX] [APM] Migrate APM from styled-components to @emotion #204222
Conversation
6168cfd
to
145d376
Compare
Pinging @elastic/obs-ux-infra_services-team (Team:obs-ux-infra_services) |
🤖 GitHub commentsExpand to view the GitHub comments
Just comment with:
|
...lity_solution/apm/public/components/app/correlations/context_popover/field_stats_popover.tsx
Outdated
Show resolved
Hide resolved
...ability_solution/apm/public/components/app/correlations/failed_transactions_correlations.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 👌🏻 I just have 3 suggestions
x-pack/plugins/observability_solution/apm/common/service_health_status.ts
Outdated
Show resolved
Hide resolved
...lity_solution/apm/public/components/app/correlations/context_popover/field_stats_popover.tsx
Outdated
Show resolved
Hide resolved
...ability_solution/apm/public/components/app/metrics/serverless_metrics/serverless_summary.tsx
Outdated
Show resolved
Hide resolved
...ck/plugins/observability_solution/apm/public/components/app/service_map/cytoscape_options.ts
Outdated
Show resolved
Hide resolved
...ck/plugins/observability_solution/apm/public/components/app/service_map/cytoscape_options.ts
Outdated
Show resolved
Hide resolved
...saction_details/waterfall_with_summary/waterfall_container/waterfall/accordion_waterfall.tsx
Outdated
Show resolved
Hide resolved
...lugins/observability_solution/apm/public/components/shared/charts/timeline/timeline_axis.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/observability_solution/apm/public/components/shared/ml_callout/index.tsx
Outdated
Show resolved
Hide resolved
iconType="apmTrace" | ||
title={ | ||
<h2> | ||
{i18n.translate('xpack.apm.transactionLink.h2.fetchingTransactionLabel', { |
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.
nice
3094308
to
9a42d79
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.
code LGTM.
edit: I tested it in dark and light mode and couldn't find anything wrong. disclaimer: my eyes are not trained to find problems with the new theme
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 all the small changes, it's a huge effort! 👏🏻
After reviewing the code, I left some comments.
I’m going to test locally now!
If I notice anything, I’ll leave another comment, otherwise, I won’t.
.../observability_solution/apm/public/components/alerting/ui_components/chart_preview/index.tsx
Outdated
Show resolved
Hide resolved
...m/public/components/routing/app_root/apm_header_action_menu/anomaly_detection_setup_link.tsx
Show resolved
Hide resolved
...lugins/observability_solution/apm/public/components/shared/kuery_bar/typeahead/suggestion.js
Outdated
Show resolved
Hide resolved
x-pack/plugins/observability_solution/apm/public/utils/test_helpers.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/observability_solution/apm/public/utils/test_helpers.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.
Good job! LGTM with only one suggestion 🚀
x-pack/plugins/observability_solution/apm/public/components/app/service_map/controls.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/observability_solution/observability_shared/public/hooks/use_chart_theme.tsx
Outdated
Show resolved
Hide resolved
x-pack/plugins/observability_solution/observability_shared/public/hooks/use_chart_theme.tsx
Outdated
Show resolved
Hide resolved
💚 Build Succeeded
Metrics [docs]Module Count
Async chunks
Page load bundle
Unknown metric groupsasync chunk count
ESLint disabled line counts
Total ESLint disabled count
History
|
/x-pack[\/\\]plugins[\/\\](observability_solution\/apm|beats_management|fleet|observability_solution\/observability|observability_solution\/observability_shared|observability_solution\/exploratory_view|security_solution|timelines|observability_solution\/synthetics|observability_solution\/ux|observability_solution\/uptime)[\/\\]/, | ||
/x-pack[\/\\]solutions[\/\\]security[\/\\]plugins[\/\\](observability_solution\/apm|beats_management|fleet|observability_solution\/infra|lists|observability_solution\/observability|observability_solution\/observability_shared|observability_solution\/exploratory_view|security_solution|timelines|observability_solution\/synthetics|observability_solution\/ux|observability_solution\/uptime)[\/\\]/, | ||
/x-pack[\/\\]plugins[\/\\](beats_management|fleet|observability_solution\/observability|observability_solution\/observability_shared|observability_solution\/exploratory_view|security_solution|timelines|observability_solution\/synthetics|observability_solution\/ux|observability_solution\/uptime)[\/\\]/, | ||
/x-pack[\/\\]solutions[\/\\]security[\/\\]plugins[\/\\](beats_management|fleet|lists|observability_solution\/observability|observability_solution\/observability_shared|observability_solution\/exploratory_view|security_solution|timelines|observability_solution\/synthetics|observability_solution\/ux|observability_solution\/uptime)[\/\\]/, |
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 this line has been messed up with the recent platform migration. I don't think there are observability plugins within security solutions.
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.
This file has been a source of conflicts since the start of the relocation of modules.
I've just created #204785 to cleanup / simplify the file and hopefully avoid more headaches.
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
/oblt-deploy |
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.
Logs changes LGMT 👏🏼
…#204222) Closes elastic#202765 ### 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 apm 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 ### Not this time There are some color values on the server side, and the values are static they would not update properly as dynamic tokens do. Eui guidance right now is to keep these as they are for now (meaning to keep using the JSON tokens). ### 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.
Closes #202765
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
styled-components
to@emotion
useEuiBackgroundColor
accentSecondary
andtextAccentSecondary
as neededNot this time
There are some color values on the server side, and the values are static they would not update properly as dynamic tokens do. Eui guidance right now is to keep these as they are for now (meaning to keep using the JSON tokens).
How to test
Running Kibana with the Borealis theme
In order to run Kibana with Borealis, you'll need to do the following: