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

[Vis Colors] Update color mapper to prioritize unique colors per vis #4890

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
3 changes: 2 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
- [Home] Add modal to introduce the `next` theme ([#4715](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/4715))
- Remove visualization editor sidebar background ([#4719](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/4719))
- [Vis Colors] Remove customized colors from sample visualizations and dashboards ([#4741](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/4741))
- [Vis Colors] Update color mapper to prioritize unique colors per visualization rather than across entire dashboard ([#4890](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/4890))
- [Decouple] Allow plugin manifest config to define semver compatible OpenSearch plugin and verify if it is installed on the cluster([#4612](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/4612))
- Adds Data explorer framework and implements Discover using it ([#4806](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/4806))

Expand Down Expand Up @@ -763,4 +764,4 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)

### 🔩 Tests

- Update caniuse to fix failed integration tests ([#2322](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/2322))
- Update caniuse to fix failed integration tests ([#2322](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/2322))
2 changes: 1 addition & 1 deletion src/plugins/charts/public/services/colors/colors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@ export class ColorsService {
) {
if (!Array.isArray(arrayOfStringsOrNumbers)) {
throw new Error(
`createColorLookupFunction expects an array but recived: ${typeof arrayOfStringsOrNumbers}`
`createColorLookupFunction expects an array but received: ${typeof arrayOfStringsOrNumbers}`
);
}

Expand Down
28 changes: 15 additions & 13 deletions src/plugins/charts/public/services/colors/mapped_colors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -83,34 +83,36 @@
const configColors = _.values(configMapping);
const oldColors = _.values(this._oldMap);

const alreadyUsedColors: string[] = [];
const keysToMap: Array<string | number> = [];
_.each(keys, (key) => {
// If this key is mapped in the config, it's unnecessary to have it mapped here
if (configMapping[key as any]) delete this._mapping[key];
if (configMapping[key as any]) {
delete this._mapping[key];
alreadyUsedColors.push(configMapping[key]);

Check warning on line 92 in src/plugins/charts/public/services/colors/mapped_colors.ts

View check run for this annotation

Codecov / codecov/patch

src/plugins/charts/public/services/colors/mapped_colors.ts#L91-L92

Added lines #L91 - L92 were not covered by tests
}

// If this key is mapped to a color used by the config color mapping, we need to remap it
if (_.includes(configColors, this._mapping[key])) keysToMap.push(key);

// if key exist in oldMap, move it to mapping
if (this._oldMap[key]) this._mapping[key] = this._oldMap[key];
if (this._oldMap[key]) {
this._mapping[key] = this._oldMap[key];
alreadyUsedColors.push(this._mapping[key]);

Check warning on line 101 in src/plugins/charts/public/services/colors/mapped_colors.ts

View check run for this annotation

Codecov / codecov/patch

src/plugins/charts/public/services/colors/mapped_colors.ts#L100-L101

Added lines #L100 - L101 were not covered by tests
}

// If this key isn't mapped, we need to map it
if (this.get(key) == null) keysToMap.push(key);
});

// Generate a color palette big enough that all new keys can have unique color values
const allColors = _(this._mapping).values().union(configColors).union(oldColors).value();
const numColors = allColors.length + keysToMap.length;
// Choose colors from euiPaletteColorBlind and filter out any already assigned to keys
const colorPalette = euiPaletteColorBlind({
rotations: Math.ceil(numColors / 10),
rotations: Math.ceil(keys.length / 10),
direction: 'both',
}).slice(0, numColors);
let newColors = _.difference(colorPalette, allColors);
})
.filter((color) => !alreadyUsedColors.includes(color.toLowerCase()))
.slice(0, keysToMap.length);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Do we need to check that after filtering, we have enough colors? as much as keysToMap.length? or we don't need to worry about it?

Copy link
Member Author

Choose a reason for hiding this comment

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

@AMoo-Miki I made this change as more of a surgical minimal change rather than a larger refactor, which this utility could probably use.

My intent is that it will always be the correct length, because we're using the total keys.length in L110 to generate the colors, and then we're only filtering out alreadyUsedColors. The intent of the L88-106 block is to push every key in keys to either alreadyUsedColors or keysToMap. But it could certainly be written cleaner to guarantee that's the case.


while (keysToMap.length > newColors.length) {
newColors = newColors.concat(_.sampleSize(allColors, keysToMap.length - newColors.length));
}

_.merge(this._mapping, _.zipObject(keysToMap, newColors));
_.merge(this._mapping, _.zipObject(keysToMap, colorPalette));
}
}
Loading