-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
[Visualize] Remove visualization:colorMapping
advanced setting
#197802
base: main
Are you sure you want to change the base?
[Visualize] Remove visualization:colorMapping
advanced setting
#197802
Conversation
visualization:colorMapping
advanced setting
5db080f
to
1b3aa5b
Compare
1b3aa5b
to
4911980
Compare
run docs-build |
Pinging @elastic/kibana-visualizations (Team:Visualizations) |
0bddc81
to
cb7e36d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
First code review pass, left some comments.
this._oldMap = {}; | ||
this._mapping = {}; | ||
} | ||
|
||
private getConfigColorMapping(): Record<string, string> { | ||
return _.mapValues(this.uiSettings?.get(COLOR_MAPPING_SETTING) || {}, standardizeColor); | ||
return _.mapValues({}, standardizeColor); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will return an empty object now.
If that's the expected result, why just return it?
return _.mapValues({}, standardizeColor); | |
return {}; |
And if so the standardizeColor
can be removed altogether.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If that is the case, I also believe the entire class can be removed, because it relies on this method to get the color for a specific key.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and probably we can also remove the syncColors
functionality that relies on this method
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe we can remove this from the dashboard (it actually doesn't work anymore as you can see from your code). I will open an issue with that and we can decide later. No need to do that in this PR.
cb7e36d
to
4ab47fc
Compare
027a6e0
to
e02560d
Compare
This comment was marked as resolved.
This comment was marked as resolved.
💚 Build Succeeded
Metrics [docs]Module Count
Public APIs missing comments
Async chunks
Public APIs missing exports
Page load bundle
History
|
Summary
Fixes #193682
Removes the deprecated
visualization:colorMapping
advanced setting.Release notes
The
visualization:colorMapping
advanced setting for TSVB and Visualize charts has been removed. Users are encouraged to switch to Lens charts, which offer a more advanced, per-chart color mapping feature with enhanced configuration options. For more details, refer to PR #162389.