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

Chart colors improvements #918

Closed
wants to merge 5 commits into from
Closed

Conversation

kzadurska
Copy link
Collaborator

What would happen if we had visualization colors in CSS? It would work, but not in the unit tests 😬

@github-actions
Copy link

size-limit report 📦

Path Size
build/public/main.js 1.73 MB (+0.01% 🔺)
build/public/polyfills.es5.js 42.74 KB (+0.01% 🔺)
build/public/dnd.es5.js 4.61 KB (0%)

"#B0B510",
"#904064"
];
export const NORMAL_COLORS = Object.values(styles);
Copy link
Contributor

Choose a reason for hiding this comment

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

Order is important here, let's map on static key names.

export type ColorScale = d3.ScaleLinear<string, string>;

const white = "#fff";
const orange = "#ff5a00";
const orange = styles.orange;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering if should we have additional "brand" colour that's somehow customisable. In Allegro case of course it would be orange and we should set orange as default.

@include css-variable(fill, item-dimension);
// @include css-variable(stroke, brand);
// @include css-variable(fill, item-dimension);
stroke: $orange;
Copy link
Contributor

Choose a reason for hiding this comment

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

Same thing with "brand" colour.

// @include css-variable(stroke, brand);
// @include css-variable(fill, item-dimension);
stroke: $orange;
fill: lighten($orange, 25%);
Copy link
Contributor

@adrianmroz-allegro adrianmroz-allegro Oct 14, 2022

Choose a reason for hiding this comment

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

That's iffy. But to be honest previous code was also iffy.

@@ -112,7 +112,7 @@ describe("Split adjustment utilities", () => {
});

describe("adjustColorSplit", () => {
it("should adjust limit with predefined limits (5, 10)", () => {
it("should adjust limit with predefined limits (5, 10)", () => { // THIS FAILS
Copy link
Contributor

Choose a reason for hiding this comment

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

That's ... surprising to say at least :o

@adrianmroz-allegro
Copy link
Contributor

Closed in favour of #955

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants