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 legacy seed colors to use ouiPaletteColorBlind() #4348

Merged
merged 1 commit into from
Jun 28, 2023
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
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,7 @@ Inspired from [Keep a Changelog](https://keepachangelog.com/en/1.0.0/)
- [Vis Colors] [Maps] Replace hardcoded color to OUI color in `maps_legacy` plugin ([#4294](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/4294))
- [Vis Colors] [TSVB] Update default color in `vis_type_timeseries` to use `ouiPaletteColorBlind()[0]`([#4363](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/4363))
- [Vis Colors] [Timeline] Replace `vis_type_timeline` colors with `ouiPaletteColorBlind()` ([#4366](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/4366))
- [Vis Colors] Update legacy seed colors to use `ouiPaletteColorBlind()` ([#4348](https://github.com/opensearch-project/OpenSearch-Dashboards/pull/4348))

### 🔩 Tests

Expand Down
38 changes: 3 additions & 35 deletions src/plugins/charts/public/services/colors/color_palette.ts
Original file line number Diff line number Diff line change
Expand Up @@ -29,48 +29,16 @@
*/

import _ from 'lodash';
import { hsl } from 'color';

import { seedColors } from './seed_colors';

const offset = 300; // Hue offset to start at

const fraction = function (goal: number) {
const walkTree = (numerator: number, denominator: number, bytes: number[]): number => {
if (bytes.length) {
return walkTree(numerator * 2 + (bytes.pop() ? 1 : -1), denominator * 2, bytes);
} else {
return numerator / denominator;
}
};

const b = (goal + 2)
.toString(2)
.split('')
.map(function (num) {
return parseInt(num, 10);
});
b.shift();

return walkTree(1, 2, b);
};
import { euiPaletteColorBlind } from '@elastic/eui';

/**
* Generates an array of hex colors the length of the input number.
* If the number is greater than the length of seed colors available,
* new colors are generated up to the value of the input number.
* Generates an array of hex colors the length of the input number
*/
export function createColorPalette(num: number): string[] {
if (!_.isNumber(num)) {
throw new TypeError('ColorPaletteUtilService expects a number');
}

const colors = seedColors;
Copy link
Member

Choose a reason for hiding this comment

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

This whole function (createColorPalette) may be able to be removed in favor of directly calling euiPaletteColorBlind(). The difference is that this function currently takes the desired number of colors as a param, while euiPaletteColorBlind takes a number of rotations instead.

So we can either:

  1. Keep createColorPalette but have it return the first num values of euiPaletteColorBlind() with a sufficient number of rotations or
  2. Just refactor callers of createColorPalette call euiPaletteColorBlind() with rotations instead of a total number of colors

I have a hunch it will be fairly quick to do 2, but am also fine with doing 1 in this PR, and 2 as a subsequent follow-up.

Don't take my word for it, but I think it's only called from

const colorPalette = createColorPalette(allColors.length + keysToMap.length);

Copy link
Member

Choose a reason for hiding this comment

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

On second thought, let's just do the easy way for now, and do the larger refactoring as part of #4320

Copy link
Member Author

Choose a reason for hiding this comment

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

Agree! createColorPalette method will be removed/refactored as part of #4320.

const seedLength = seedColors.length;

_.times(num - seedLength, function (i) {
colors.push(hsl((fraction(i + seedLength + 1) * 360 + offset) % 360, 50, 50).hex());
});

return colors;
return euiPaletteColorBlind({ rotations: Math.ceil(num / 10), direction: 'both' }).slice(0, num);
}
4 changes: 2 additions & 2 deletions src/plugins/charts/public/services/colors/colors.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@

import { coreMock } from '../../../../../core/public/mocks';
import { COLOR_MAPPING_SETTING } from '../../../common';
import { seedColors } from './seed_colors';
import { euiPaletteColorBlind } from '@elastic/eui';
import { ColorsService } from './colors';

// Local state for config
Expand Down Expand Up @@ -138,7 +138,7 @@ describe('Vislib Color Service', () => {
});

it('should return the first hex color in the seed colors array', () => {
expect(color(arr[0])).toBe(seedColors[0]);
expect(color(arr[0])).toBe(euiPaletteColorBlind()[0]);
});

it('should return the value from the mapped colors', () => {
Expand Down
4 changes: 2 additions & 2 deletions src/plugins/charts/public/services/colors/colors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,8 +32,8 @@ import _ from 'lodash';

import { CoreSetup } from 'opensearch-dashboards/public';

import { euiPaletteColorBlind } from '@elastic/eui';
import { MappedColors } from './mapped_colors';
import { seedColors } from './seed_colors';

/**
* Accepts an array of strings or numbers that are used to create a
Expand All @@ -44,7 +44,7 @@ import { seedColors } from './seed_colors';
export class ColorsService {
private _mappedColors?: MappedColors;

public readonly seedColors = seedColors;
public readonly seedColors = euiPaletteColorBlind();
Copy link
Member

Choose a reason for hiding this comment

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

I wonder if we even need to provide this in the color service? Because consumers could also just get them directly from OUI. I suppose we can tackle that question once we do some of the other color refactoring and consolidation...

Copy link
Member

Choose a reason for hiding this comment

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

@manasvinibs Can we make a follow-up issue to deprecate this public API and refactor the only known usage of it:

colors: ChartsPluginSetup['colors'];

and
this._tagCloud = new TagCloud(cloudContainer, d3.scale.ordinal().range(colors.seedColors));

as documented in:

2. `ChartsPluginSetup['colors']` used by `vis_type_tagcloud` plugin. Only the seed colors are used via `d3.scale.ordinal().range(colors.seedColors)`

Copy link
Member Author

Choose a reason for hiding this comment

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


public get mappedColors() {
if (!this._mappedColors) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,8 @@
* under the License.
*/

import { seedColors } from './seed_colors';
import { createColorPalette } from './color_palette';
import { euiPaletteColorBlind } from '@elastic/eui';

describe('Color Palette', () => {
const num1 = 45;
Expand All @@ -46,6 +46,12 @@ describe('Color Palette', () => {
colorPalette = createColorPalette(num1);
});

function isHexValue(value: string): boolean {
// Check if the hex value is valid.
const regex = /^#[0-9a-fA-F]{3}|[0-9a-fA-F]{6}$/;
return regex.test(value) ? true : false;
}

it('should throw an error if input is not a number', () => {
expect(() => {
// @ts-expect-error
Expand Down Expand Up @@ -91,18 +97,21 @@ describe('Color Palette', () => {
});

it('should return the seed color array when input length is 72', () => {
expect(createColorPalette(num2)[71]).toBe(seedColors[71]);
expect(createColorPalette(num2).length).toBe(num2);
expect(isHexValue(createColorPalette(num2)[71])).toBe(true);
});

it('should return an array of the same length as the input when input is greater than 72', () => {
expect(createColorPalette(num3).length).toBe(num3);
});

it('should create new darker colors when input is greater than 72', () => {
expect(createColorPalette(num3)[72]).not.toEqual(seedColors[0]);
expect(createColorPalette(num3)[72]).not.toEqual(euiPaletteColorBlind()[0]);
});

it('should create new colors and convert them correctly', () => {
expect(createColorPalette(num3)[72]).toEqual('#404ABF');
expect(createColorPalette(num3).length).toBe(num3);
expect(createColorPalette(num3)[72]).not.toEqual(euiPaletteColorBlind()[9]);
expect(isHexValue(createColorPalette(num3)[89])).toBe(true);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ import Color from 'color';

import { coreMock } from '../../../../../core/public/mocks';
import { COLOR_MAPPING_SETTING } from '../../../common';
import { seedColors } from './seed_colors';
import { euiPaletteColorBlind } from '@elastic/eui';
import { MappedColors } from './mapped_colors';

// Local state for config
Expand Down Expand Up @@ -65,31 +65,31 @@ describe('Mapped Colors', () => {
});

it('should not include colors used by the config', () => {
const newConfig = { bar: seedColors[0] };
const newConfig = { bar: euiPaletteColorBlind()[9] };
config.set(COLOR_MAPPING_SETTING, newConfig);

const arr = ['foo', 'baz', 'qux'];
mappedColors.mapKeys(arr);

const colorValues = _(mappedColors.mapping).values();
expect(colorValues).not.toContain(seedColors[0]);
expect(colorValues).not.toContain(euiPaletteColorBlind()[9]);
expect(colorValues.uniq().size()).toBe(arr.length);
});

it('should create a unique array of colors even when config is set', () => {
const newConfig = { bar: seedColors[0] };
const newConfig = { bar: euiPaletteColorBlind()[9] };
config.set(COLOR_MAPPING_SETTING, newConfig);

const arr = ['foo', 'bar', 'baz', 'qux'];
mappedColors.mapKeys(arr);

const expectedSize = _(arr).difference(_.keys(newConfig)).size();
expect(_(mappedColors.mapping).values().uniq().size()).toBe(expectedSize);
expect(mappedColors.get(arr[0])).not.toBe(seedColors[0]);
expect(mappedColors.get(arr[0])).not.toBe(euiPaletteColorBlind()[9]);
});

it('should treat different formats of colors as equal', () => {
const color = new Color(seedColors[0]);
const color = new Color(euiPaletteColorBlind()[9]);
const rgb = `rgb(${color.red()}, ${color.green()}, ${color.blue()})`;
const newConfig = { bar: rgb };
config.set(COLOR_MAPPING_SETTING, newConfig);
Expand All @@ -99,8 +99,8 @@ describe('Mapped Colors', () => {

const expectedSize = _(arr).difference(_.keys(newConfig)).size();
expect(_(mappedColors.mapping).values().uniq().size()).toBe(expectedSize);
expect(mappedColors.get(arr[0])).not.toBe(seedColors[0]);
expect(mappedColors.get('bar')).toBe(seedColors[0]);
expect(mappedColors.get(arr[0])).not.toBe(euiPaletteColorBlind()[9]);
expect(mappedColors.get('bar')).toBe(euiPaletteColorBlind()[9].toLowerCase());
});

it('should have a flush method that moves the current map to the old map', function () {
Expand Down
37 changes: 0 additions & 37 deletions src/plugins/charts/public/services/colors/seed_colors.test.ts

This file was deleted.

44 changes: 0 additions & 44 deletions src/plugins/charts/public/services/colors/seed_colors.ts

This file was deleted.

6 changes: 3 additions & 3 deletions test/functional/apps/dashboard/dashboard_state.js
Original file line number Diff line number Diff line change
Expand Up @@ -277,14 +277,14 @@ export default function ({ getService, getPageObjects }) {

await retry.try(async () => {
const pieSliceStyle = await pieChart.getPieSliceStyle('80,000');
// The default green color that was stored with the visualization before any dashboard overrides.
expect(pieSliceStyle.indexOf('rgb(87, 193, 123)')).to.be.greaterThan(0);
// The default color that was stored with the visualization before any dashboard overrides.
expect(pieSliceStyle.indexOf('rgb(84, 179, 153)')).to.be.greaterThan(0);
});
});

it('resets the legend color as well', async function () {
await retry.try(async () => {
const colorExists = await PageObjects.visChart.doesSelectedLegendColorExist('#57c17b');
const colorExists = await PageObjects.visChart.doesSelectedLegendColorExist('#54B399');
Comment on lines +280 to +287
Copy link
Member

Choose a reason for hiding this comment

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

@manasvinibs Can you open a follow-up issue to refactor/improve these tests to be less brittle?

Copy link
Member Author

Choose a reason for hiding this comment

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

oh yes I already have the issue for that #4424

expect(colorExists).to.be(true);
});
});
Expand Down