-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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
[Dashboard] Deprecate syncColors
option
#200276
Conversation
🤖 Jobs for this PR can be triggered through checkboxes. 🚧
ℹ️ To trigger the CI, please tick the checkbox below 👇
|
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.
@nreese I moved these changes to a separate PR for further discussion...
Map embeddable still uses syncColors. Should this be cleaned up as part of this PR?
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.
Looking at the Maps code usage of syncColors
I don't think it's using it. Maybe you can help.
Here you are setting up the syncing by setting the chartsPaletteServiceGetColor
method to the store when the option is enabled.
kibana/x-pack/plugins/maps/public/react_embeddable/initialize_redux_sync.ts
Lines 154 to 165 in 53dd55b
if (syncColors$) { | |
syncColorsSubscription = syncColors$.subscribe(async (syncColors: boolean | undefined) => { | |
const currentSyncColorsSymbol = Symbol(); | |
syncColorsSymbol = currentSyncColorsSymbol; | |
const chartsPaletteServiceGetColor = syncColors | |
? await getChartsPaletteServiceGetColor() | |
: null; | |
if (syncColorsSymbol === currentSyncColorsSymbol) { | |
store.dispatch(setChartsPaletteServiceGetColor(chartsPaletteServiceGetColor)); | |
} | |
}); | |
} |
Here is the function that creates the getColor
logic.
kibana/x-pack/plugins/maps/public/react_embeddable/initialize_redux_sync.ts
Lines 263 to 279 in 53dd55b
async function getChartsPaletteServiceGetColor(): Promise<((value: string) => string) | null> { | |
const chartsService = getCharts(); | |
const paletteRegistry: PaletteRegistry | null = chartsService | |
? await chartsService.palettes.getPalettes() | |
: null; | |
if (!paletteRegistry) { | |
return null; | |
} | |
const paletteDefinition = paletteRegistry.get('default'); | |
const chartConfiguration = { syncColors: true }; | |
return (value: string) => { | |
const series = [{ name: value, rankAtDepth: 0, totalSeriesAtDepth: 1 }]; | |
const color = paletteDefinition.getCategoricalColor(series, chartConfiguration); | |
return color ? color : '#3d3d3d'; | |
}; | |
} |
But the strange thing is if I enable the setting and put a breakpoint on line 277
in the getColor
callback, it's never called when the map is rendered. 🤔
Furthermore, if I create a dashboard with 2 charts and 2 maps, one of each filters out a value to have a different legend orders. Enabling the syncColors
option. I see the charts sync colors for like series names, but the maps do not.
Am I missing something? What calls the chartsPaletteServiceGetColor
function?
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.
Follow these steps to see sync colors effect map
- install sample web logs
- create new map
- add documents layer from
Kibana Sample Data Logs
- Under scaling, select "Limit results to 10000"
- Under "Layer style" -> "Fill color", select "by value" and select field "geo.dest".
- Save map to library
- Create new dashboard and add map.
- Under dashboard settings, change toggle "sync colors" on and hit Apply. Notice how the colors shift in the legend.
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.
Oh I see, so it only works with the scaling set to Limit results to 10000
. Looks to work for most things but breaks Lens Pie charts. 😅
Do you have a strong opinion to keep this feature around? I think there are too many caveats to make it work as a typical user would expect (only specific palettes under certain settings). Also it would help if we deprecated this as we plan to move away from the legacy palette service and towards the new color mapping.
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.
Do you have a strong opinion to keep this feature around?
I am fine removing as part of 9.0. Just want to make sure the removal also cleans up maps.
I think there are too many caveats to make it work as a typical user would expect (only specific palettes under certain settings).
Agree. Most users will be using vector tiles which do not support sync colors. And this is not even documented.
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 added a discuss item to our team sync on Tuesday morning to get feedback from team. I will invite you to the meeting.
I also opened #200290 to track removing sync colors from maps since it is non-trivial.
💔 Build Failed
Failed CI StepsHistory |
syncColors
option, remove related fn testssyncColors
option, remove related fn tests
syncColors
option, remove related fn testssyncColors
option
Closing this for now until we explore alternative functionality, see #200795. |
Summary
This PR deprecates the usage of
colorSync
option in the dashboard.Closes #200272
Checklist
release_note:breaking
label should be applied in these situations.release_node:*
label is applied per the guidelines