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

Fixing color assignment order #2

Draft
wants to merge 1 commit into
base: fix/make-other-color-consistent
Choose a base branch
from
Draft

Fixing color assignment order #2

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
39 changes: 23 additions & 16 deletions ...lugins/shared/chart_expressions/expression_partition_vis/public/utils/layers/get_color.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,28 +145,29 @@ const createSeriesLayers = (
formatters: Record<string, FieldFormat | undefined>,
formatter: FieldFormatsStart,
column: Partial<BucketColumns>,
chartType: ChartTypes
chartType: ChartTypes,
colorIndexMap: Map<string, number>
): SeriesLayer[] => {
const seriesLayers: SeriesLayer[] = [];
let tempParent: typeof arrayNode | (typeof arrayNode)['parent'] = arrayNode;
const groupsRemainingAsOther = Boolean(
tempParent?.parent?.children?.find((child) => child[0] === '__other__')
);
// const groupsRemainingAsOther = Boolean(
// tempParent?.parent?.children?.find((child) => child[0] === '__other__')
// );

while (tempParent.parent && tempParent.depth > 0) {
const nodeKey = tempParent.parent.children[tempParent.sortIndex][0];
const seriesName = String(nodeKey);

let sortIndex = tempParent.sortIndex;
// let sortIndex = tempParent.sortIndex;

if (chartType === ChartTypes.MOSAIC && groupsRemainingAsOther) {
const newIndex = tempParent.sortIndex + 1;
if (newIndex > tempParent.parent?.children?.length - 1) {
sortIndex = 0;
} else {
sortIndex = newIndex;
}
}
// if (chartType === ChartTypes.MOSAIC && groupsRemainingAsOther) {
// const newIndex = tempParent.sortIndex + 1;
// if (newIndex > tempParent.parent?.children?.length - 1) {
// sortIndex = 0;
// } else {
// sortIndex = newIndex;
// }
// }

/**
* FIXME this is a bad implementation: The `parentSeries` is an array of both `string` and `RangeKey` even if its type
Expand All @@ -176,14 +177,15 @@ const createSeriesLayers = (
*/
const isSplitParentLayer = isSplitChart && parentSeries.includes(seriesName);
const formattedName = getNodeLabel(nodeKey, column, formatters, formatter.deserialize);
const colorIndex = colorIndexMap.get(seriesName) ?? tempParent.sortIndex;
seriesLayers.unshift({
// by construction and types `formattedName` should be always be a string, but I leave this Nullish Coalescing
// because I don't trust much our formatting functions
name: formattedName ?? seriesName,
rankAtDepth: isSplitParentLayer
? // FIXME as described above this will not work correctly if the `nodeKey` is a `RangeKey`
parentSeries.findIndex((name) => name === seriesName)
: sortIndex,
: colorIndex,
totalSeriesAtDepth: isSplitParentLayer
? parentSeries.length
: tempParent.parent.children.length,
Expand Down Expand Up @@ -230,7 +232,8 @@ export const getColor = (
isDarkMode: boolean,
formatter: FieldFormatsStart,
column: Partial<BucketColumns>,
formatters: Record<string, FieldFormat | undefined>
formatters: Record<string, FieldFormat | undefined>,
colorIndexMap: Map<string, number>
) => {
// Mind the difference here: the contrast computation for the text ignores the alpha/opacity
// therefore change it for dark mode
Expand All @@ -255,14 +258,18 @@ export const getColor = (
);
}

const colorIndex = colorIndexMap.get(categoricalKey);
console.log(colorIndex);

const seriesLayers = createSeriesLayers(
arrayNode,
distinctSeries.parentSeries,
isSplitChart,
formatters,
formatter,
column,
chartType
chartType,
colorIndexMap
);

const overriddenColor = overrideColors(seriesLayers, overwriteColors, name);
Expand Down
10 changes: 9 additions & 1 deletion ...ugins/shared/chart_expressions/expression_partition_vis/public/utils/layers/get_layers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,13 @@ export const getLayers = (
isDarkMode
);

const categories =
chartType === ChartTypes.MOSAIC && columns.length === 2
? getColorCategories(rows, columns[1]?.id)
: getColorCategories(rows, columns[0]?.id);

const colorIndexMap = new Map(categories.map((c, i) => [String(c), i]));

return columns.map((col, layerIndex) => {
return {
groupByRollup: (d: Datum) => (col.id ? d[col.id] ?? emptySliceLabel : col.name),
Expand Down Expand Up @@ -113,7 +120,8 @@ export const getLayers = (
isDarkMode,
formatter,
col,
formatters
formatters,
colorIndexMap
),
},
};
Expand Down
7 changes: 5 additions & 2 deletions ...s/shared/chart_expressions/expression_partition_vis/public/utils/layers/sort_predicate.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,11 @@ export const sortPredicateSaveSourceOrder: SortPredicatePureFn =
return node2.value - node1.value;
};

const sortPredicatePieDonut: SortPredicatePieDonutFn = (visParams) =>
visParams.respectSourceOrder ? sortPredicateSaveSourceOrder() : undefined;
const sortPredicatePieDonut: SortPredicatePieDonutFn = (visParams) => {
const test = visParams.respectSourceOrder ? sortPredicateSaveSourceOrder() : undefined;

return test;
};

const sortPredicateMosaic: SortPredicateDefaultFn = (visData, columns) => {
const sortingMap = columns[0]?.id ? extractUniqTermsMap(visData, columns[0].id) : {};
Expand Down