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

[Observability plugin] Audit new EUI Borealis theme #204234

Closed
wants to merge 10 commits into from
Closed

[Observability plugin] Audit new EUI Borealis theme #204234

wants to merge 10 commits into from

Conversation

fkanout
Copy link
Contributor

@fkanout fkanout commented Dec 13, 2024 •
edited
Loading

Summary

It fixes #203338

Note

Some of the tokens have been ignored as they were on the server side or common folder, or they are coming from a shared package like packages/react/kibana_context

@fkanout fkanout self-assigned this Dec 16, 2024
@fkanout fkanout added the Team:obs-ux-management Observability Management User Experience Team label Dec 16, 2024
@fkanout fkanout requested review from kkurstak and patpscal December 16, 2024 13:00
@fkanout fkanout marked this pull request as ready for review December 16, 2024 13:43
@fkanout fkanout requested review from a team as code owners December 16, 2024 13:43
@elasticmachine
Copy link
Contributor

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

@fkanout fkanout added v9.0.0 backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) release_note:skip Skip the PR/issue when compiling release notes labels Dec 16, 2024
@fkanout
Copy link
Contributor Author

fkanout commented Dec 16, 2024

/oblt-deploy

kdelemme
kdelemme reviewed Dec 16, 2024
View reviewed changes
Copy link
Contributor

@kdelemme kdelemme left a comment

Choose a reason for hiding this comment

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

A few questions but otherwise token changes LGTM.

x-pack/plugins/observability_solution/apm/common/viz_colors.ts Show resolved Hide resolved
...k/plugins/observability_solution/profiling/public/components/topn_functions/function_row.tsx Outdated Show resolved Hide resolved
...ervability_solution/infra/public/components/asset_details/tabs/processes/processes_table.tsx Outdated Show resolved Hide resolved
...ins/synthetics/public/apps/synthetics/components/common/monitor_test_result/status_badge.tsx Outdated Show resolved Hide resolved
@botelastic botelastic bot added ci:project-deploy-observability Create an Observability project Team:obs-ux-infra_services Observability Infrastructure & Services User Experience Team labels Dec 16, 2024
@elasticmachine
Copy link
Contributor

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

@github-actions GitHub Actions
Copy link
Contributor

🤖 GitHub comments

Expand to view the GitHub comments

Just comment with:

  • /oblt-deploy : Deploy a Kibana instance using the Observability test environments.
  • run docs-build : Re-trigger the docs validation. (use unformatted text in the comment!)

mgadewoll
mgadewoll reviewed Dec 17, 2024
View reviewed changes
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.

@fkanout We need to add the label backport:skip and remove backport:prev-minor.
Borealis changes should not be backported to 8.x versions.

x-pack/plugins/observability_solution/apm/common/service_health_status.ts Outdated
switch (status) {
case ServiceHealthStatus.healthy:
return theme.eui.euiColorVis0;
return theme.colors.vis.euiColorVis0;
Copy link
Contributor

Choose a reason for hiding this comment

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

Fyi, vis colors changed. So especially for severity colors like this it means using the same mapping will change colors in Amsterdam. This might need alignment with a designer (not sure which is responsible here?), if that's expected or not.

See this related conversation about this topic.

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, thanks for the heads up! @maciejforcone is our designer, but I don't think we can get his feedback soon enough for the PR due to the holiday period.

However, I chose the 1st option in the related conversation you mentioned. a.k.a using euiColorVisSuccess0, euiColorVisWarning0, and euiColorVisDanger0

@jasonrhodes please let me know if you think we need to do otherwise.

Copy link
Contributor

Choose a reason for hiding this comment

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

Fyi, we're introducing new severity colors that should cover those cases across Kibana.
We can update to severity colors as follow-up.

x-pack/plugins/observability_solution/apm/public/components/shared/charts/timeseries_chart.tsx Show resolved Hide resolved
@fkanout fkanout added backport:skip This commit does not require backporting and removed backport:prev-minor Backport to (8.x) the previous minor version (i.e. one version back from main) labels Dec 17, 2024
@elasticmachine
Copy link
Contributor

elasticmachine commented Dec 17, 2024 •
edited
Loading

💔 Build Failed

  • Buildkite Build
  • Commit: aac8ef4
  • Kibana Serverless Image: docker.elastic.co/kibana-ci/kibana-serverless:pr-204234-aac8ef418b16

Failed CI Steps

Test Failures

  • [job] [logs] FTR Configs #80 / Cloud Security Posture Findings Page onboarding Vulnerabilities - clicking on the No integrations installed prompt action button - install CNVM: navigates to the CNVM integration installation page
  • [job] [logs] Jest Tests #16 / EmptyBanner with one node does not render null
  • [job] [logs] Jest Tests #16 / EmptyBanner with one node does not render null
  • [job] [logs] Jest Tests #16 / Timeline TimelineAxisContainer should render with data
  • [job] [logs] Jest Tests #16 / Timeline TimelineAxisContainer should render with data
  • [job] [logs] Jest Tests #16 / Timeline VerticalLinesContainer should render with data
  • [job] [logs] Jest Tests #16 / Timeline VerticalLinesContainer should render with data
  • [job] [logs] Jest Tests #16 / useCytoscapeEventHandlers when data is received runs the layout
  • [job] [logs] Jest Tests #16 / useCytoscapeEventHandlers when data is received runs the layout
  • [job] [logs] Jest Tests #16 / useCytoscapeEventHandlers when data is received with a service name sets the primary class
  • [job] [logs] Jest Tests #16 / useCytoscapeEventHandlers when data is received with a service name sets the primary class

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
apm 1922 1929 +7

Async chunks

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

id before after diff
apm 3.3MB 3.4MB +9.6KB
infra 1.7MB 1.7MB -78.0B
synthetics 1.1MB 1.1MB +18.0B
total +9.6KB

History

cc @fkanout

@fkanout
Copy link
Contributor Author

fkanout commented Dec 17, 2024

@kdelemme @mgadewoll This PR seems to overlap with other ones scope-wise. I discussed this with @jasonrhodes, and we agreed to scope this PR to update only the files that are owned by our team.
Sorry about letting you double-review this work. But the new PR is significantly smaller. I appreciate your help!

@patpscal
Copy link

Hi @fkanout! To make our design review as helpful as possible, could you share from a final user's perspective where they might notice the changes (in case it impacts the user)?

@fkanout
Copy link
Contributor Author

fkanout commented Dec 18, 2024

closing this in favor of #204615

@fkanout fkanout closed this Dec 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mgadewoll mgadewoll mgadewoll left review comments

@kdelemme kdelemme kdelemme left review comments

@kkurstak kkurstak Awaiting requested review from kkurstak

@patpscal patpscal Awaiting requested review from patpscal

Assignees

@fkanout fkanout

Labels
backport:skip This commit does not require backporting ci:project-deploy-observability Create an Observability project release_note:skip Skip the PR/issue when compiling release notes Team:obs-ux-infra_services Observability Infrastructure & Services User Experience Team Team:obs-ux-management Observability Management User Experience Team v9.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Observability plugin] Audit new EUI Borealis theme
5 participants
@fkanout @elasticmachine @patpscal @kdelemme @mgadewoll