[Vis Colors] Update legacy seed colors to use ouiPaletteColorBlind()

[manasvinibs]

Description

As part of #4318, this PR will remove hardcoded seed colors and replace its usages with OUI color palettes.

Issues Resolved

#4319

Screenshots:

Tag cloud Vis for the sample data

Check List

• All tests pass
  • yarn test:jest
  • yarn test:jest_integration
  • yarn test:ftr
• New functionality includes testing.
• New functionality has been documented.
• Update CHANGELOG.md
• Commits are signed per the DCO using --signoff

[codecov]

Codecov Report

Merging #4348 (a70d4b7) into main (a44b09f) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

❗ Current head a70d4b7 differs from pull request most recent head f268e0c. Consider uploading reports for the commit f268e0c to get more accurate results

@@            Coverage Diff             @@
##             main    #4348      +/-   ##
==========================================
- Coverage   66.41%   66.40%   -0.01%     
==========================================
  Files        3246     3245       -1     
  Lines       62475    62459      -16     
  Branches     9674     9671       -3     
==========================================
- Hits        41493    41479      -14     
+ Misses      18634    18633       -1     
+ Partials     2348     2347       -1     

Flag	Coverage Δ
Linux	66.35% <100.00%> (-0.01%)	⬇️
Windows	66.35% <100.00%> (-0.01%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
...ins/charts/public/services/colors/color_palette.ts	100.00% <100.00%> (ø)
...rc/plugins/charts/public/services/colors/colors.ts	100.00% <100.00%> (ø)

... and 5 files with indirect coverage changes

[joshuarrrr]

@manasvinibs Great start!

Just a couple comments about further reducing some of the un-needed color abstractions. And we should avoid hard-coded colors in tests because we know they are subject to change from the OUI side (except in the cases where we're validating user color selection overrides).

[joshuarrrr]

lol, this test wasn't providing a lot...

[joshuarrrr]

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

OpenSearch-Dashboards/src/plugins/charts/public/services/colors/mapped_colors.ts

Line 103 in 84ec854

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

[joshuarrrr]

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

[manasvinibs]

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

[joshuarrrr]

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...

[joshuarrrr]

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

OpenSearch-Dashboards/src/plugins/vis_type_tagcloud/public/plugin.ts

Line 57 in fc4e696

colors: ChartsPluginSetup['colors'];

and

OpenSearch-Dashboards/src/plugins/vis_type_tagcloud/public/components/tag_cloud_visualization.js

Line 61 in fc4e696

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

as documented in:

OpenSearch-Dashboards/docs/charts/current_usage.md

Line 14 in fc4e696

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

[manasvinibs]

#4406

[joshuarrrr]

@manasvinibs Can you also provide some screenshots of affected visualizations? We particularly want to see any of the tag clouds included in the sample datasets.

[manasvinibs]

@manasvinibs Can you also provide some screenshots of affected visualizations? We particularly want to see any of the tag clouds included in the sample datasets.

Tag cloud vis screenshot added in the description.

[joshuarrrr]

let's remove this in favor just making sure we get enough colors from euiPaletteColorBlind() so we can just return a slice of those directly without any additional transformation.

[manasvinibs]

oh I see! Sorry I thought we will do any modification to this function as part of #4320. Ya I think we can get rid of this transformation logic and just use rotations on oui palette to fill the array. For now I've used 10 rotations, total of 100 colors which seems to be sufficient.

[joshuarrrr]

@manasvinibs There's a unit test failing due to a casing discrepancy:

FAIL  src/plugins/charts/public/services/colors/colors.test.ts
  ● Vislib Color Service › should return the first hex color in the seed colors array

    expect(received).toBe(expected) // Object.is equality

    Expected: "#54B399"
    Received: "#54b399"

      139 |
      140 |   it('should return the first hex color in the seed colors array', () => {
    > 141 |     expect(color(arr[0])).toBe(euiPaletteColorBlind()[0]);
          |                           ^
      142 |   });
      143 |
      144 |   it('should return the value from the mapped colors', () => {

      at Object.<anonymous> (src/plugins/charts/public/services/colors/colors.test.ts:141:27)


Test Suites: 1 failed, 1 skipped, 1527 passed, 1528 of 1529 total
Tests:       1 failed, 31 skipped, 9 todo, 12025 passed, 12066 total
Snapshots:   2917 passed, 2917 total
Time:        3074.378 s
Ran all test suites.
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

[manasvinibs]

@manasvinibs There's a unit test failing due to a casing discrepancy:

FAIL  src/plugins/charts/public/services/colors/colors.test.ts
  ● Vislib Color Service › should return the first hex color in the seed colors array

    expect(received).toBe(expected) // Object.is equality

    Expected: "#54B399"
    Received: "#54b399"

      139 |
      140 |   it('should return the first hex color in the seed colors array', () => {
    > 141 |     expect(color(arr[0])).toBe(euiPaletteColorBlind()[0]);
          |                           ^
      142 |   });
      143 |
      144 |   it('should return the value from the mapped colors', () => {

      at Object.<anonymous> (src/plugins/charts/public/services/colors/colors.test.ts:141:27)


Test Suites: 1 failed, 1 skipped, 1527 passed, 1528 of 1529 total
Tests:       1 failed, 31 skipped, 9 todo, 12025 passed, 12066 total
Snapshots:   2917 passed, 2917 total
Time:        3074.378 s
Ran all test suites.
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

Thanks for catching that! I'm sure I saw that before and thought I fixed it, but it was different test file.

[BSFishy]

This may chop off the colors in a weird way that doesn't grab colors from the whole palette. It may be better to drag the number of rotations to the minimum number it can be so as much of the palette is used.

const colors = euiPaletteColorBlind({ rotations: Math.ceil(num / 10) * 10 });

[manasvinibs]

Do you mean const colors = euiPaletteColorBlind({ rotations: Math.ceil(num / 10) }); ?

But then isn't the use case is to return color palette array of size equal to num? If we don't slice, we will be sending color palette with size little more than num in few cases. Do we know if we can eliminate the test case which is testing the equality condition?

[joshuarrrr]

I do prefer calculating the rotations from the num rather than hard-coding. But I agree that we should return slice() to return an array of length num. For this use case, it's not a problem that not all colors are used from the final rotation.

[BSFishy]

You're right, effectively it would be euiPaletteColorBlind({ rotations: Math.ceil(num / 10) }).slice(0, num)

[joshuarrrr]

why is it [1] instead of [0] - does this test target the second pie slice?

[manasvinibs]

Sorry it has to be RGB format of colors and also I think it can be any color in the default color palette.

[joshuarrrr]

OK, let's roll with this. I would like to have a follow-up issue for either migrating or improving tests that rely on specific color values - we'd like those to be more robust to future theming updates.

[joshuarrrr]

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

[manasvinibs]

oh yes I already have the issue for that #4424