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

Optimize charts plugin #78922

Merged
merged 4 commits into from
Sep 30, 2020
Merged

Optimize charts plugin #78922

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
1cd1bf9
Optimize charts plugin
timroes Sep 30, 2020
c2e98bc
Fix issues to keep same behavior
timroes Sep 30, 2020
fb58b89
Remove dead import
timroes Sep 30, 2020
85f50b7
Revert name change
timroes Sep 30, 2020
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
6 changes: 3 additions & 3 deletions src/plugins/charts/public/services/colors/color_palette.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,8 @@
* under the License.
*/

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

import { seedColors } from './seed_colors';

Expand Down Expand Up @@ -49,7 +49,7 @@ const fraction = function (goal: 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.
*/
export function createColorPalette(num?: any): string[] {
export function createColorPalette(num: number): string[] {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ℹ️ I also improved the TypeScript code in that place here and got rid of the any.

if (!_.isNumber(num)) {
throw new TypeError('ColorPaletteUtilService expects a number');
}
Expand All @@ -58,7 +58,7 @@ export function createColorPalette(num?: any): string[] {
const seedLength = seedColors.length;

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

return colors;
Expand Down
6 changes: 6 additions & 0 deletions src/plugins/charts/public/services/colors/colors_palette.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,26 +37,32 @@ describe('Color Palette', () => {

it('should throw an error if input is not a number', () => {
expect(() => {
// @ts-expect-error
createColorPalette(string);
}).toThrowError();

expect(() => {
// @ts-expect-error
createColorPalette(bool);
}).toThrowError();

expect(() => {
// @ts-expect-error
createColorPalette(nullValue);
}).toThrowError();

expect(() => {
// @ts-expect-error
createColorPalette(emptyArr);
}).toThrowError();

expect(() => {
// @ts-expect-error
createColorPalette(emptyObject);
}).toThrowError();

expect(() => {
// @ts-expect-error
createColorPalette();
}).toThrowError();
});
Expand Down
8 changes: 4 additions & 4 deletions src/plugins/charts/public/services/colors/mapped_colors.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
*/

import _ from 'lodash';
import d3 from 'd3';
import Color from 'color';

import { coreMock } from '../../../../../core/public/mocks';
import { COLOR_MAPPING_SETTING } from '../../../common';
Expand Down Expand Up @@ -61,7 +61,7 @@ describe('Mapped Colors', () => {
mappedColors.mapKeys(arr);

const colorValues = _(mappedColors.mapping).values();
expect(colorValues.includes(seedColors[0])).toBe(false);
expect(colorValues).not.toContain(seedColors[0]);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ℹ️ A left over from debugging the tests, since this produces way better output, but I'd just leave it with this clearer API.

expect(colorValues.uniq().size()).toBe(arr.length);
});

Expand All @@ -78,8 +78,8 @@ describe('Mapped Colors', () => {
});

it('should treat different formats of colors as equal', () => {
const color = d3.rgb(seedColors[0]);
const rgb = `rgb(${color.r}, ${color.g}, ${color.b})`;
const color = new Color(seedColors[0]);
const rgb = `rgb(${color.red()}, ${color.green()}, ${color.blue()})`;
const newConfig = { bar: rgb };
config.set(COLOR_MAPPING_SETTING, newConfig);

Expand Down
4 changes: 2 additions & 2 deletions src/plugins/charts/public/services/colors/mapped_colors.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,14 +18,14 @@
*/

import _ from 'lodash';
import d3 from 'd3';
import Color from 'color';

import { CoreSetup } from 'kibana/public';

import { COLOR_MAPPING_SETTING } from '../../../common';
import { createColorPalette } from './color_palette';

const standardizeColor = (color: string) => d3.rgb(color).toString();
const standardizeColor = (color: string) => new Color(color).hex().toLowerCase();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

ℹ️ The old code used d3.toString() which created a lower-case hex value, to make sure I don't need to touch the tests and we can thus better guarantee that this doesn't change any functionality, I kept here the same logic, even though technically we could use .hex() as well and just adjust the tests.


/**
* Maintains a lookup table that associates the value (key) with a hex color (value)
Expand Down