Skip to content

[Visual Refresh] behindText datavis and severity color cleanup#206061

Merged
mgadewoll merged 2 commits intoelastic:mainfrom
mgadewoll:eui/162-163-behindText-and-severity-color-cleanup
Jan 21, 2025
Merged

[Visual Refresh] behindText datavis and severity color cleanup#206061
mgadewoll merged 2 commits intoelastic:mainfrom
mgadewoll:eui/162-163-behindText-and-severity-color-cleanup

Conversation

@mgadewoll
Copy link
Copy Markdown
Contributor

@mgadewoll mgadewoll commented Jan 9, 2025

Summary

This PR adds cleanups to previously merged Borealis-related updates to usages of severity colors and/or behindText data vis colors as EUI provides matching tokens and initial guidance on usage by now (see point 5) in this FAQ for the guidance).
These updates ensure that we keep parity for Amsterdam colors by using conditional updates while using semantic tokens for Borealis.

Note

Please let me know in case your team already made those changes or has other plans for the code.

ℹ️ Management changes were done separately by the team itself here.


Checklist

Check the PR satisfies following conditions.

Reviewers should verify this PR satisfies this list as well.

  • Any text added follows EUI's writing guidelines, uses sentence case text and includes i18n support
  • Documentation was added for features that require explanation or tutorials
  • Unit or functional tests were updated or added to match the most common scenarios
  • If a plugin configuration key changed, check if it needs to be allowlisted in the cloud and added to the docker list
  • This was checked for breaking HTTP API changes, and any breaking changes have been approved by the breaking-change committee. The release_note:breaking label should be applied in these situations.
  • Flaky Test Runner was used on any tests changed
  • The PR description includes the appropriate Release Notes section, and the correct release_note:* label is applied per the guidelines

Identify risks

Does this PR introduce any risks? For example, consider risks like hard to test bugs, performance regression, potential of data loss.

Describe the risk, its severity, and mitigation for each identified risk. Invite stakeholders and evaluate how to proceed before merging.

@mgadewoll mgadewoll added release_note:skip Skip the PR/issue when compiling release notes EUI backport:skip This PR does not require backporting EUI Visual Refresh labels Jan 9, 2025
@mgadewoll mgadewoll force-pushed the eui/162-163-behindText-and-severity-color-cleanup branch from 9fe0a01 to a5ce42e Compare January 13, 2025 14:48
@mgadewoll mgadewoll marked this pull request as ready for review January 14, 2025 12:42
@mgadewoll mgadewoll requested a review from a team as a code owner January 14, 2025 12:42
@mgadewoll mgadewoll requested a review from a team January 14, 2025 12:42
@mgadewoll mgadewoll requested review from a team as code owners January 14, 2025 12:42
@elasticmachine
Copy link
Copy Markdown
Contributor

Pinging @elastic/eui-team (EUI)

@mgadewoll mgadewoll force-pushed the eui/162-163-behindText-and-severity-color-cleanup branch from a5ce42e to f47b5d6 Compare January 14, 2025 17:15
@botelastic botelastic bot added Team:Fleet Team label for Observability Data Collection Fleet team Team:obs-ux-infra_services - DEPRECATED DEPRECATED - Use Team:obs-presentation. Team:actionable-obs Formerly "obs-ux-management", responsible for SLO, o11y alerting, significant events, & synthetics. labels Jan 14, 2025
@elasticmachine
Copy link
Copy Markdown
Contributor

Pinging @elastic/fleet (Team:Fleet)

@elasticmachine
Copy link
Copy Markdown
Contributor

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

@elasticmachine
Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
Contributor

@MiriamAparicio MiriamAparicio left a comment

Choose a reason for hiding this comment

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

Changes in obs-ux-infra_services LGTM, thanks for the cleanup 🌟

@mgadewoll mgadewoll force-pushed the eui/162-163-behindText-and-severity-color-cleanup branch from f47b5d6 to e7478b0 Compare January 15, 2025 13:23
Copy link
Copy Markdown
Contributor

@criamico criamico left a comment

Choose a reason for hiding this comment

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

I tested Fleet changes locally, LGTM

Copy link
Copy Markdown
Contributor

@walterra walterra left a comment

Choose a reason for hiding this comment

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

ML related changes LGTM, tested the colors on both themes for NER tests.

@mgadewoll
Copy link
Copy Markdown
Contributor Author

mgadewoll commented Jan 20, 2025

@elasticmachine merge upstream

@mgadewoll
Copy link
Copy Markdown
Contributor Author

@elastic/obs-ux-management-team and @elastic/kibana-operations could you have a look as well please? 🙂

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This spot seems to have a different way of checking if the theme is indeed Amsterdam.

I can see how these could be equivalent, but it stands out, maybe it's worth following the pattern with others?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yeah they mean the same and both have been used, but I agree that it's better to align on one. I'll update to use themeName instead as it's more straight forward. 👍

- reverts Amsterdam colors of previously changed usages by using conditions; uses new available tokens
@mgadewoll mgadewoll force-pushed the eui/162-163-behindText-and-severity-color-cleanup branch from 2ffb37a to 5b09b7d Compare January 21, 2025 13:44
@mgadewoll mgadewoll enabled auto-merge (squash) January 21, 2025 13:45
@mgadewoll mgadewoll merged commit 8050089 into elastic:main Jan 21, 2025
@elasticmachine
Copy link
Copy Markdown
Contributor

💛 Build succeeded, but was flaky

Failed CI Steps

Test Failures

  • [job] [logs] FTR Configs #45 / console app console app tabs navigation tabs should be navigable through URL

Metrics [docs]

Async chunks

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

id before after diff
apm 4.0MB 4.0MB +736.0B
fleet 1.7MB 1.7MB +158.0B
ml 4.8MB 4.8MB -19.0B
synthetics 917.5KB 917.6KB +124.0B
uptime 461.4KB 461.6KB +200.0B
total +1.2KB

History

  • 💚 Build #268322 succeeded 2ffb37a3c307025c4d02351ea9d22efc3789632c
  • 💔 Build #267361 failed e7478b049c5ebf6256080e6ff76237a93d433b3f
  • 💔 Build #267082 failed f47b5d695dfad6a70d9c21cf7e7f5fb3df1dd354
  • 💚 Build #266559 succeeded a5ce42e3e8f20619b618c8a74087b274bd3f8d90
  • 💔 Build #265615 failed 9fe0a019cb1278f88fcbe81dc866a13c5d1a583b

viduni94 pushed a commit to viduni94/kibana that referenced this pull request Jan 23, 2025
…ic#206061)

## Summary

This PR adds cleanups to previously merged Borealis-related updates to
usages of severity colors and/or `behindText` data vis colors as EUI
provides matching tokens and initial guidance on usage by now (see point
5) in this
[FAQ](elastic#199715 (comment))
for the guidance).
These updates ensure that we keep parity for Amsterdam colors by using
conditional updates while using semantic tokens for Borealis.

>[!NOTE]
Please let me know in case your team already made those changes or has
other plans for the code.

ℹ️ Management changes were done separately by the team itself
[here](elastic#206026).

---

### Checklist

Check the PR satisfies following conditions. 

Reviewers should verify this PR satisfies this list as well.

- [ ] Any text added follows [EUI's writing
guidelines](https://elastic.github.io/eui/#/guidelines/writing), uses
sentence case text and includes [i18n
support](https://github.com/elastic/kibana/blob/main/src/platform/packages/shared/kbn-i18n/README.md)
- [ ]
[Documentation](https://www.elastic.co/guide/en/kibana/master/development-documentation.html)
was added for features that require explanation or tutorials
- [ ] [Unit or functional
tests](https://www.elastic.co/guide/en/kibana/master/development-tests.html)
were updated or added to match the most common scenarios
- [ ] If a plugin configuration key changed, check if it needs to be
allowlisted in the cloud and added to the [docker
list](https://github.com/elastic/kibana/blob/main/src/dev/build/tasks/os_packages/docker_generator/resources/base/bin/kibana-docker)
- [ ] This was checked for breaking HTTP API changes, and any breaking
changes have been approved by the breaking-change committee. The
`release_note:breaking` label should be applied in these situations.
- [ ] [Flaky Test
Runner](https://ci-stats.kibana.dev/trigger_flaky_test_runner/1) was
used on any tests changed
- [ ] The PR description includes the appropriate Release Notes section,
and the correct `release_note:*` label is applied per the
[guidelines](https://www.elastic.co/guide/en/kibana/master/contributing.html#kibana-release-notes-process)

### Identify risks

Does this PR introduce any risks? For example, consider risks like hard
to test bugs, performance regression, potential of data loss.

Describe the risk, its severity, and mitigation for each identified
risk. Invite stakeholders and evaluate how to proceed before merging.

- [ ] [See some risk
examples](https://github.com/elastic/kibana/blob/main/RISK_MATRIX.mdx)
- [ ] ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport:skip This PR does not require backporting EUI Visual Refresh EUI release_note:skip Skip the PR/issue when compiling release notes Team:actionable-obs Formerly "obs-ux-management", responsible for SLO, o11y alerting, significant events, & synthetics. Team:Fleet Team label for Observability Data Collection Fleet team Team:obs-ux-infra_services - DEPRECATED DEPRECATED - Use Team:obs-presentation. v9.0.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants