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.

Sign up for GitHub

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

Jump to bottom

Refactor usage of sparkline chart styles #179503

Merged
merged 15 commits into from
Apr 10, 2024
Merged

Refactor usage of sparkline chart styles #179503

merged 15 commits into from
Apr 10, 2024

Conversation

nickofthyme
Copy link
Contributor

@nickofthyme nickofthyme commented Mar 27, 2024 •
edited
Loading

Summary

Refactor usage of EUI_SPARKLINE_THEME_PARTIAL from eui across kibana.

  • add useSparklineOverrides hook to charts plugin theme service
  • refactor usages across kibana
  • refactor trigger action ui alert summary chart theme logic to read baseTheme directly from local charts plugin within the trigger_action_ui plugin instead of where the component is used, passed as ChartProps.
  • Rename ChartProps.theme from alert_summary_widget to themeOverrides

Checklist

@nickofthyme
refactor usage of sparkline chart styles
a439dfe 
- remove usage of `EUI_SPARKLINE_THEME_PARTIAL` from eui
- add `useSparklineOverrides` hook to charts.theme plugin
- refactor usages across kibana
- refactor trigger action ui alert summary to read base theme from charts.theme, rename theme props to themeOverrides
@nickofthyme nickofthyme added Feature:ElasticCharts Issues related to the elastic-charts library release_note:skip Skip the PR/issue when compiling release notes labels Mar 27, 2024
@nickofthyme nickofthyme mentioned this pull request Mar 27, 2024
Merged
4 tasks
@nickofthyme nickofthyme marked this pull request as ready for review March 29, 2024 23:02
@nickofthyme nickofthyme requested review from a team as code owners March 29, 2024 23:02
@botelastic botelastic bot added the Team:obs-ux-management Observability Management User Experience Team label Apr 1, 2024
@elasticmachine
Copy link
Contributor

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

yngrdyn
yngrdyn approved these changes Apr 2, 2024
View reviewed changes
Copy link
Contributor

@yngrdyn yngrdyn left a comment

Choose a reason for hiding this comment

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

obs-ux-logs changes LGTM

cauemarcondes
cauemarcondes approved these changes Apr 2, 2024
View reviewed changes
Copy link
Contributor

@cauemarcondes cauemarcondes left a comment

Choose a reason for hiding this comment

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

LGTM

maryam-saeidi
maryam-saeidi reviewed Apr 5, 2024
View reviewed changes
Copy link
Member

@maryam-saeidi maryam-saeidi left a comment

Choose a reason for hiding this comment

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

Overall, LGTM, nice improvement! ✨

I just added one clarification question to our discussion to make sure I understand how the current implementation works.

...on/observability/public/pages/overview/components/sections/metrics/metric_with_sparkline.tsx Show resolved Hide resolved
x-pack/plugins/triggers_actions_ui/public/application/sections/alert_summary_widget/types.ts Show resolved Hide resolved
maryam-saeidi
maryam-saeidi approved these changes Apr 9, 2024
View reviewed changes
Copy link
Member

@maryam-saeidi maryam-saeidi left a comment

Choose a reason for hiding this comment

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

Awesome job! 💯

@nickofthyme nickofthyme enabled auto-merge (squash) April 10, 2024 15:03
@kibana-ci
Copy link
Collaborator

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] FTR Configs #42 / apis transform basic license transform /internal/transform/reauthorize_transforms single transform reauthorize_transforms "after each" hook for "should reauthorize transform created by transform_viewer with new api key of poweruser and start the transform"

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
infra 1454 1455 +1
observability 505 506 +1
total +2

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id before after diff
infra 1.4MB 1.4MB +54.0B
observability 282.0KB 281.9KB -99.0B
slo 611.0KB 610.9KB -101.0B
triggersActionsUi 1.6MB 1.6MB +153.0B
total +7.0B

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
charts 44.8KB 44.9KB +164.0B
infra 102.5KB 102.4KB -65.0B
observability 150.4KB 150.3KB -65.0B
slo 21.4KB 21.3KB -65.0B
triggersActionsUi 116.7KB 116.8KB +42.0B
total +11.0B

History

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

@nickofthyme nickofthyme merged commit 53af24e into elastic:main Apr 10, 2024
20 checks passed
@nickofthyme nickofthyme deleted the chart-sparkline-theme branch April 10, 2024 16:20
@kibanamachine kibanamachine added v8.14.0 backport:skip This commit does not require backporting labels Apr 10, 2024
@nickofthyme nickofthyme mentioned this pull request Apr 12, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@XavierM XavierM XavierM approved these changes

@yngrdyn yngrdyn yngrdyn approved these changes

@markov00 markov00 markov00 approved these changes

@maryam-saeidi maryam-saeidi maryam-saeidi approved these changes

@cauemarcondes cauemarcondes cauemarcondes approved these changes

Assignees
No one assigned
Labels
apm:review backport:skip This commit does not require backporting Feature:ElasticCharts Issues related to the elastic-charts library release_note:skip Skip the PR/issue when compiling release notes Team:obs-ux-management Observability Management User Experience Team v8.14.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@nickofthyme @elasticmachine @kibana-ci @XavierM @yngrdyn @markov00 @maryam-saeidi @cauemarcondes @smith @kibanamachine