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

[Lens] Fix partition chart color assignments #207178

Merged
merged 17 commits into from
Mar 4, 2025
Merged

[Lens] Fix partition chart color assignments #207178

merged 17 commits into from
Mar 4, 2025

Conversation

kowalczyk-krzysztof
Copy link
Member

@kowalczyk-krzysztof kowalczyk-krzysztof commented Jan 20, 2025 •
edited by kibanamachine
Loading

Summary

This PR fixes the color assignment of all partition charts consistent with the legend ordering.

Fixes #202813
Fixes #202816

@kowalczyk-krzysztof kowalczyk-krzysztof added bug Fixes for quality problems that affect the customer experience Team:Visualizations Visualization editors, elastic-charts and infrastructure release_note:skip Skip the PR/issue when compiling release notes Feature:Lens backport:prev-minor Backport to (9.0) the previous minor version (i.e. one version back from main) labels Jan 20, 2025
@kowalczyk-krzysztof kowalczyk-krzysztof self-assigned this Jan 20, 2025
@kowalczyk-krzysztof kowalczyk-krzysztof requested a review from a team as a code owner January 20, 2025 11:41
@elasticmachine
Copy link
Contributor

Pinging @elastic/kibana-visualizations (Team:Visualizations)

@kowalczyk-krzysztof kowalczyk-krzysztof marked this pull request as draft January 31, 2025 03:07
@nickofthyme nickofthyme self-requested a review January 31, 2025 16:11
@kowalczyk-krzysztof
Copy link
Member Author

@nickofthyme
I added all the changes we talked about and I'm trying to solve the two edge cases with treemap and mosaic when there are two fields added (the "white" color thing). It seems that 7c24a0e solves it for treemap and it kinda solves it for mosaic too - although the non-legacy color mapping feature is broken for mosaic.

nickofthyme
nickofthyme reviewed Feb 4, 2025
View reviewed changes
Copy link
Contributor

@nickofthyme nickofthyme left a comment

Choose a reason for hiding this comment

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

@kowalczyk-krzysztof This section of the code is the legacy palettes and there is a reason we want to get rid of it. So I just want to say it's not you it's the code!

I took a lot at the code and found a solution. I try to explain the parts below but if you could take a look, update the tests and test other edge cases you can find.

@nickofthyme nickofthyme changed the title [Lens] Make Other color for partition charts consistent [Lens] Fix partition chart color assignment order Feb 4, 2025
@kowalczyk-krzysztof
Copy link
Member Author

@nickofthyme
Thanks for your help. The code definitely lives up to the "legacy" name 😅 I'll take a closer look at your proposed changes, play with them, see if anything else breaks and I'll fix any tests.

@kowalczyk-krzysztof
Copy link
Member Author

@nickofthyme
So I played around with partition charts and I couldn't notice anything wrong visually except for Mosaic having top layer as white in both legacy and new color palette. Is this intentional?

I tried fixing the tests (src/platform/plugins/shared/chart_expressions/expression_partition_vis/public/utils/layers/get_color.test.ts) but I'm getting some failures but I'm not knowledgeable enough on this to know if those are actual failures or I'm doing something wrong with changing those tests.

nickofthyme
nickofthyme reviewed Feb 5, 2025
View reviewed changes
Copy link
Contributor

@nickofthyme nickofthyme left a comment

Choose a reason for hiding this comment

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

Ok I took another look at the changes and found a few issues. I updated the tests and tweaked a few final things on the src file. Take a look and lmk what you think.

Ping me tomorrow if you'd like me to walk through the changes with you.

@kowalczyk-krzysztof kowalczyk-krzysztof marked this pull request as ready for review February 5, 2025 11:30
nickofthyme
nickofthyme approved these changes Mar 4, 2025
View reviewed changes
Copy link
Contributor

@nickofthyme nickofthyme left a comment

Choose a reason for hiding this comment

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

Changes LGTM

@nickofthyme nickofthyme requested a review from markov00 March 4, 2025 03:40
markov00
markov00 approved these changes Mar 4, 2025
View reviewed changes
Copy link
Member

@markov00 markov00 left a comment

Choose a reason for hiding this comment

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

Tested locally, it fixes the mentioned issues.LGTM

@nickofthyme nickofthyme enabled auto-merge (squash) March 4, 2025 15:25
@nickofthyme nickofthyme disabled auto-merge March 4, 2025 15:25
@nickofthyme nickofthyme enabled auto-merge (squash) March 4, 2025 15:27
@nickofthyme nickofthyme changed the title [Lens] Fix partition chart color assignment order [Lens] Fix partition chart color assignments Mar 4, 2025
@nickofthyme nickofthyme merged commit 28dc0f6 into elastic:main Mar 4, 2025
10 checks passed
@elasticmachine
Copy link
Contributor

💛 Build succeeded, but was flaky

Failed CI Steps

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
expressionPartitionVis 36.7KB 36.7KB +72.0B
Unknown metric groups

References to deprecated APIs

id before after diff
expressionPartitionVis 8 7 -1

History

cc @kowalczyk-krzysztof

@kibanamachine
Copy link
Contributor

Starting backport for target branches: 9.0

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

kibanamachine pushed a commit to kibanamachine/kibana that referenced this pull request Mar 4, 2025
@kowalczyk-krzysztof
[Lens] Fix partition chart color assignments (elastic#207178)
c5e5334 
Fixes the color assignment for partition charts consistent with the legend ordering. Aligns legacy and color mapping color logic.

(cherry picked from commit 28dc0f6)
@kibanamachine kibanamachine mentioned this pull request Mar 4, 2025
Merged
@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 Mar 4, 2025
@kibanamachine @kowalczyk-krzysztof
[9.0] [Lens] Fix partition chart color assignments (#207178) (#213149)
029cde7 
# Backport

This will backport the following commits from `main` to `9.0`:
- [[Lens] Fix partition chart color assignments
(#207178)](#207178)

<!--- Backport version: 9.6.6 -->

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

<!--BACKPORT [{"author":{"name":"Krzysztof
Kowalczyk","email":"krzysztof.kowalczyk@elastic.co"},"sourceCommit":{"committedDate":"2025-03-04T17:20:43Z","message":"[Lens]
Fix partition chart color assignments (#207178)\n\nFixes the color
assignment for partition charts consistent with the legend ordering.
Aligns legacy and color mapping color
logic.","sha":"28dc0f6ffcf82279dc3fcaee477b4727b8bbdd0a","branchLabelMapping":{"^v9.1.0$":"main","^v8.19.0$":"8.x","^v(\\d+).(\\d+).\\d+$":"$1.$2"}},"sourcePullRequest":{"labels":["bug","Team:Visualizations","release_note:skip","Feature:Lens","backport:prev-minor","v9.1.0"],"title":"[Lens]
Fix partition chart color
assignments","number":207178,"url":"https://github.com/elastic/kibana/pull/207178","mergeCommit":{"message":"[Lens]
Fix partition chart color assignments (#207178)\n\nFixes the color
assignment for partition charts consistent with the legend ordering.
Aligns legacy and color mapping color
logic.","sha":"28dc0f6ffcf82279dc3fcaee477b4727b8bbdd0a"}},"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/207178","number":207178,"mergeCommit":{"message":"[Lens]
Fix partition chart color assignments (#207178)\n\nFixes the color
assignment for partition charts consistent with the legend ordering.
Aligns legacy and color mapping color
logic.","sha":"28dc0f6ffcf82279dc3fcaee477b4727b8bbdd0a"}}]}]
BACKPORT-->

Co-authored-by: Krzysztof Kowalczyk <krzysztof.kowalczyk@elastic.co>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@timductive timductive timductive left review comments

@markov00 markov00 markov00 approved these changes

@nickofthyme nickofthyme nickofthyme approved these changes

Assignees

@kowalczyk-krzysztof kowalczyk-krzysztof

Labels
backport:prev-minor Backport to (9.0) the previous minor version (i.e. one version back from main) bug Fixes for quality problems that affect the customer experience Feature:Lens release_note:skip Skip the PR/issue when compiling release notes Team:Visualizations Visualization editors, elastic-charts and infrastructure v9.0.0 v9.1.0
Projects
None yet
Milestone
No milestone
6 participants
@kowalczyk-krzysztof @elasticmachine @kibanamachine @timductive @markov00 @nickofthyme