cal.com icon indicating copy to clipboard operation
cal.com copied to clipboard

[CAL-1603] Replace hacky color functions in getBrandColor.ts

Open sean-brydon opened this issue 1 year ago • 9 comments

Currently all the functions here are written from scratch

There is a library which makes this file a lot easier to read and not as complicated if we ever need to debug it.

https://polished.js.org/docs/

From SyncLinear.com | CAL-1603

sean-brydon avatar Apr 29 '23 10:04 sean-brydon

I want to work on this issue plz assign this to me

ghost avatar Apr 29 '23 14:04 ghost

Hi @sean-brydon , I would love to work on this issue. My approach to solving the issue will be as follows:

  1. Importing the package in getBrandedColors.tsx file
  2. Replacing the functions written from scratch with functions provided by polished package in the colors sections (link) as follows
function rgbToHex(r: number, g: number, b: number): string  // can be replaced to ⬇️

rgb(value: (RgbColor | number) , green: number?, blue: number?): string

// the rgb function provided by the package takes RGB values as params and returns hex valued string

  • Similarly, the package has functions to lighten and darken the color with respect to amount provided along with the RGB color

  • The package has a function getLuminance which returns the luminescence as used in the code multiple times.

  • The code has a function - checkWCAGContrastColor() which first finds the contrast color based on 2 colors passed as strings. This finding of contrast ration can be replaced by getContrast(color1: string, color2:string): number function.

Apart from above functions, I felt that other functions are core functions and can't be replaced by the library functions. If there are any, then I will surely replace as you say!

Can you assign this issue to me. Thank you!

SohamRatnaparkhi avatar Apr 29 '23 18:04 SohamRatnaparkhi

Hi @sean-brydon , I would love to work on this issue. My approach to solving the issue will be as follows:

  1. Importing the package in getBrandedColors.tsx file
  2. Replacing the functions written from scratch with functions provided by polished package in the colors sections (link) as follows
function rgbToHex(r: number, g: number, b: number): string  // can be replaced to ⬇️

rgb(value: (RgbColor | number) , green: number?, blue: number?): string

// the rgb function provided by the package takes RGB values as params and returns hex valued string
  • Similarly, the package has functions to lighten and darken the color with respect to amount provided along with the RGB color
  • The package has a function getLuminance which returns the luminescence as used in the code multiple times.
  • The code has a function - checkWCAGContrastColor() which first finds the contrast color based on 2 colors passed as strings. This finding of contrast ration can be replaced by getContrast(color1: string, color2:string): number function.

Apart from above functions, I felt that other functions are core functions and can't be replaced by the library functions. If there are any, then I will surely replace as you say!

Can you assign this issue to me. Thank you!

Thanks @SohamRatnaparkhi - I've passed this over to you. The lighten/darken/rgb were the ones i was on about. Nice spot on the getContrast too!

Please give me a shout if you get stuck!

sean-brydon avatar Apr 30 '23 11:04 sean-brydon

Thanks @SohamRatnaparkhi - I've passed this over to you. The lighten/darken/rgb were the ones i was on about. Nice spot on the getContrast too! Please give me a shout if you get stuck!

Cool! Will make a PR soon.

SohamRatnaparkhi avatar Apr 30 '23 14:04 SohamRatnaparkhi

Hi @sean-brydon My PR fails because it doesn't pass the tests for branding colors. On checking as to why it doesn't pass, I found out that the library functions return completely different light and dark colors based on the intensity. Results with respect to the test in the brandedColors.test.ts file -> link

With respect to lightMap:

Light Map check
expected - #fff3f3 got - #fff
expected - #ffe8e8 got - #fff
expected - #ffc5c5 got - #fff
expected - #ffa2a2 got - #fff
expected - #ff5c5c got - #ffafaf
matches - #ff1616
expected - #e61414 got - #000
expected - #bf1111 got - #000
expected - #990d0d got - #000
expected - #7d0b0b got - #1b0000

With respect to dark map:

Dark Map check
expected - #f2f5ff got - #fff
expected - #e6ecff got - #fff
expected - #bfcfff got - #fff
expected - #99b3ff got - #fff
expected - #4d79ff got - #99b3ff
matches - #0040ff
expected - #003ae6 got - #000
expected - #0030bf got - #000
expected - #002699 got - #000
expected - #001f7d got - #000105

To generate these results, I had made a short test script locally. the hex-code following the word expected is the hexcode according to the expectedResultsMap and hex-code following the word got is the code that the library functions return.

SohamRatnaparkhi avatar Apr 30 '23 19:04 SohamRatnaparkhi

Hi @sean-brydon My PR fails because it doesn't pass the tests for branding colors. On checking as to why it doesn't pass, I found out that the library functions return completely different light and dark colors based on the intensity. Results with respect to the test in the brandedColors.test.ts file -> link

With respect to lightMap:

Light Map check
expected - #fff3f3 got - #fff
expected - #ffe8e8 got - #fff
expected - #ffc5c5 got - #fff
expected - #ffa2a2 got - #fff
expected - #ff5c5c got - #ffafaf
matches - #ff1616
expected - #e61414 got - #000
expected - #bf1111 got - #000
expected - #990d0d got - #000
expected - #7d0b0b got - #1b0000

With respect to dark map:

Dark Map check
expected - #f2f5ff got - #fff
expected - #e6ecff got - #fff
expected - #bfcfff got - #fff
expected - #99b3ff got - #fff
expected - #4d79ff got - #99b3ff
matches - #0040ff
expected - #003ae6 got - #000
expected - #0030bf got - #000
expected - #002699 got - #000
expected - #001f7d got - #000105

To generate these results, I had made a short test script locally. the hex-code following the word expected is the hexcode according to the expectedResultsMap and hex-code following the word got is the code that the library functions return.

Interesting - Thanks for giving this a shot. Us at Cal.com are currently on a retreat to improve team working skills.

I will see if i can fix this based on what you have contributed here. Weird how it doesnt work here

sean-brydon avatar May 02 '23 10:05 sean-brydon

Okay cool! Ping me if required.

SohamRatnaparkhi avatar May 02 '23 15:05 SohamRatnaparkhi

This is super weird - I can't figure out why we dont get the expected results as they do the same thing

sean-brydon avatar May 04 '23 10:05 sean-brydon

This is super weird - I can't figure out why we dont get the expected results as they do the same thing

I think, they are doing something different from what we do. Link to lighten code

By the way, I am sharing down my testing script below.


const { rgb, lighten, darken , getContrast, getLuminance } = require("polished");

const lightColor = '#ff1616'
const darkColor = '#0040ff'


const createColorMap = (brandColor) => {
    const response = {
    500: `#${brandColor}`.replace("##", "#"),
    };
    const intensityMap = {
    50: 0.95,
    100: 0.9,
    200: 0.75,
    300: 0.6,
    400: 0.3,
    600: 0.9,
    700: 0.75,
    800: 0.6,
    900: 0.49,
    };

    [50, 100, 200, 300, 400].forEach((level) => {
    response[level] = lighten(intensityMap[level], brandColor);
    });

    [600, 700, 800, 900].forEach((level) => {
    response[level] = darken(intensityMap[level], brandColor);
    });
    return response;
};

const lightMap = createColorMap("#ff1616");
const darkMap = createColorMap("#0040ff");

const expectedResult = {
      light: {
        "50": "#fff3f3",
        "100": "#ffe8e8",
        "200": "#ffc5c5",
        "300": "#ffa2a2",
        "400": "#ff5c5c",
        "500": "#ff1616",
        "600": "#e61414",
        "700": "#bf1111",
        "800": "#990d0d",
        "900": "#7d0b0b",
      },
      dark: {
        "50": "#f2f5ff",
        "100": "#e6ecff",
        "200": "#bfcfff",
        "300": "#99b3ff",
        "400": "#4d79ff",
        "500": "#0040ff",
        "600": "#003ae6",
        "700": "#0030bf",
        "800": "#002699",
        "900": "#001f7d",
      },
      // ...
    };

// check for difference

console.log("Light Map check")
for (const key in expectedResult.light) {
    if (expectedResult.light.hasOwnProperty(key)) {
        const element = expectedResult.light[key];
        console.log(element !== lightMap[key] ? "expected - " + element + " got - " + lightMap[key] : "matches - " + element)
    }
}

console.log("\nDark Map check")
for (const key in expectedResult.dark) {
    if (expectedResult.dark.hasOwnProperty(key)) {
        const element = expectedResult.dark[key];
        console.log(element !== darkMap[key] ? "expected - " + element + " got - " + darkMap[key] : "matches - " + element)
    }
}

console.log(expectedResult.light === lightMap)
console.log(expectedResult.dark === darkMap)

SohamRatnaparkhi avatar May 04 '23 13:05 SohamRatnaparkhi