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.

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] Adjust timeseries charts colors #210749

Conversation

MiriamAparicio
Copy link
Contributor

@MiriamAparicio MiriamAparicio commented Feb 12, 2025

Summary

Because of the new order of colors within the list of data vis colors, we need to alternate colors when loading them in sequence in the absence of the definition of series colors overrides. This will avoid confusion between charts that are very close in color.

For the same reasons, we might want to review and modify some of the values ascribed to the specific ChartType in get_timeseries_color.ts

The bug with the charts being the same colors was introduced in this PR

BEFORE

image

AFTER

image

image

image

@MiriamAparicio MiriamAparicio added release_note:skip Skip the PR/issue when compiling release notes backport:prev-minor Backport to (9.0) the previous minor version (i.e. one version back from main) Team:obs-ux-infra_services Observability Infrastructure & Services User Experience Team v9.1.0 labels Feb 12, 2025
@MiriamAparicio MiriamAparicio requested a review from a team as a code owner February 12, 2025 08:43
@elasticmachine
Copy link
Contributor

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

@jennypavlova jennypavlova self-requested a review February 12, 2025 08:58
Copy link
Member

@jennypavlova jennypavlova left a comment

Choose a reason for hiding this comment

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

LGTM 🚀 The charts colors on the Overview/Transaction/Errors/Dependencies/ are different colors now and it's nice to have the red scheme for errors/failures 🙌
Q: I see the metrics colors are not changed but I guess this is not part of the scope of this PR right (they are different charts as well)?
image

@MiriamAparicio
Copy link
Contributor Author

Thanks for the review @jennypavlova

I see the metrics colors are not changed but I guess this is not part of the scope of this PR right (they are different charts as well)?

Those charts use Lens, Lens decided the colors there AFAIK

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

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 +367.0B

Copy link

@patpscal patpscal left a comment

Choose a reason for hiding this comment

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

Looks much better and more recognizable

@MiriamAparicio MiriamAparicio merged commit d6db880 into elastic:main Feb 12, 2025
18 checks passed
@MiriamAparicio MiriamAparicio deleted the 383-adjust-colors-time-series-charts branch February 12, 2025 11:24
@kibanamachine
Copy link
Contributor

Starting backport for target branches: 9.0

https://github.com/elastic/kibana/actions/runs/13284251414

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Feb 12, 2025
## Summary

Because of the new order of colors within the list of data vis colors,
we need to alternate colors when loading them in sequence in the absence
of the definition of series colors overrides. This will avoid confusion
between charts that are very close in color.

For the same reasons, we might want to review and modify some of the
values ascribed to the specific ChartType in `get_timeseries_color.ts`

The bug with the charts being the same colors was introduced in this
[PR](elastic#207070)

BEFORE

![image](https://github.com/user-attachments/assets/e8ba0797-0ae4-429c-8a43-a6688c3d1b12)

AFTER

![image](https://github.com/user-attachments/assets/bec05099-7c56-47ea-937a-34b85b953f89)

![image](https://github.com/user-attachments/assets/c38556ec-a8cf-4708-a557-de74ab268f6b)

![image](https://github.com/user-attachments/assets/e7f56647-be11-4453-9e49-9c0df607f728)

(cherry picked from commit d6db880)
@kibanamachine
Copy link
Contributor

💚 All backports created successfully

Status Branch Result
9.0

Note: Successful backport PRs will be merged automatically after passing CI.

Questions ?

Please refer to the Backport tool documentation

kibanamachine added a commit that referenced this pull request Feb 12, 2025
# Backport

This will backport the following commits from `main` to `9.0`:
- [[ObsUX][APM] Adjust timeseries charts colors
(#210749)](#210749)

<!--- Backport version: 9.4.3 -->

### Questions ?
Please refer to the [Backport tool
documentation](https://github.com/sqren/backport)

<!--BACKPORT
[{"author":{"name":"Miriam","email":"[email protected]"},"sourceCommit":{"committedDate":"2025-02-12T11:24:01Z","message":"[ObsUX][APM]
Adjust timeseries charts colors (#210749)\n\n## Summary\r\n\r\nBecause
of the new order of colors within the list of data vis colors,\r\nwe
need to alternate colors when loading them in sequence in the
absence\r\nof the definition of series colors overrides. This will avoid
confusion\r\nbetween charts that are very close in color.\r\n\r\nFor the
same reasons, we might want to review and modify some of the\r\nvalues
ascribed to the specific ChartType in
`get_timeseries_color.ts`\r\n\r\nThe bug with the charts being the same
colors was introduced in
this\r\n[PR](https://github.com/elastic/kibana/pull/207070)\r\n\r\nBEFORE\r\n\r\n\r\n![image](https://github.com/user-attachments/assets/e8ba0797-0ae4-429c-8a43-a6688c3d1b12)\r\n\r\nAFTER\r\n\r\n\r\n![image](https://github.com/user-attachments/assets/bec05099-7c56-47ea-937a-34b85b953f89)\r\n\r\n\r\n![image](https://github.com/user-attachments/assets/c38556ec-a8cf-4708-a557-de74ab268f6b)\r\n\r\n\r\n![image](https://github.com/user-attachments/assets/e7f56647-be11-4453-9e49-9c0df607f728)","sha":"d6db8802da215410be9117339b1e2be07bbfb4cd","branchLabelMapping":{"^v9.1.0$":"main","^v8.19.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["release_note:skip","backport:prev-minor","Team:obs-ux-infra_services","v9.1.0"],"title":"[ObsUX][APM]
Adjust timeseries charts
colors","number":210749,"url":"https://github.com/elastic/kibana/pull/210749","mergeCommit":{"message":"[ObsUX][APM]
Adjust timeseries charts colors (#210749)\n\n## Summary\r\n\r\nBecause
of the new order of colors within the list of data vis colors,\r\nwe
need to alternate colors when loading them in sequence in the
absence\r\nof the definition of series colors overrides. This will avoid
confusion\r\nbetween charts that are very close in color.\r\n\r\nFor the
same reasons, we might want to review and modify some of the\r\nvalues
ascribed to the specific ChartType in
`get_timeseries_color.ts`\r\n\r\nThe bug with the charts being the same
colors was introduced in
this\r\n[PR](https://github.com/elastic/kibana/pull/207070)\r\n\r\nBEFORE\r\n\r\n\r\n![image](https://github.com/user-attachments/assets/e8ba0797-0ae4-429c-8a43-a6688c3d1b12)\r\n\r\nAFTER\r\n\r\n\r\n![image](https://github.com/user-attachments/assets/bec05099-7c56-47ea-937a-34b85b953f89)\r\n\r\n\r\n![image](https://github.com/user-attachments/assets/c38556ec-a8cf-4708-a557-de74ab268f6b)\r\n\r\n\r\n![image](https://github.com/user-attachments/assets/e7f56647-be11-4453-9e49-9c0df607f728)","sha":"d6db8802da215410be9117339b1e2be07bbfb4cd"}},"sourceBranch":"main","suggestedTargetBranches":[],"targetPullRequestStates":[{"branch":"main","label":"v9.1.0","branchLabelMappingKey":"^v9.1.0$","isSourceBranch":true,"state":"MERGED","url":"https://github.com/elastic/kibana/pull/210749","number":210749,"mergeCommit":{"message":"[ObsUX][APM]
Adjust timeseries charts colors (#210749)\n\n## Summary\r\n\r\nBecause
of the new order of colors within the list of data vis colors,\r\nwe
need to alternate colors when loading them in sequence in the
absence\r\nof the definition of series colors overrides. This will avoid
confusion\r\nbetween charts that are very close in color.\r\n\r\nFor the
same reasons, we might want to review and modify some of the\r\nvalues
ascribed to the specific ChartType in
`get_timeseries_color.ts`\r\n\r\nThe bug with the charts being the same
colors was introduced in
this\r\n[PR](https://github.com/elastic/kibana/pull/207070)\r\n\r\nBEFORE\r\n\r\n\r\n![image](https://github.com/user-attachments/assets/e8ba0797-0ae4-429c-8a43-a6688c3d1b12)\r\n\r\nAFTER\r\n\r\n\r\n![image](https://github.com/user-attachments/assets/bec05099-7c56-47ea-937a-34b85b953f89)\r\n\r\n\r\n![image](https://github.com/user-attachments/assets/c38556ec-a8cf-4708-a557-de74ab268f6b)\r\n\r\n\r\n![image](https://github.com/user-attachments/assets/e7f56647-be11-4453-9e49-9c0df607f728)","sha":"d6db8802da215410be9117339b1e2be07bbfb4cd"}}]}]
BACKPORT-->

Co-authored-by: Miriam <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:prev-minor Backport to (9.0) the previous minor version (i.e. one version back from main) release_note:skip Skip the PR/issue when compiling release notes Team:obs-ux-infra_services Observability Infrastructure & Services User Experience Team v9.0.0 v9.1.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants