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

[BD-46] docs: update color picker readme #2646

Closed
wants to merge 1 commit into from

Conversation

khudym
Copy link
Contributor

@khudym khudym commented Sep 22, 2023

Description

Issue

Deploy Preview

Preview
Include a direct link to your changes in this PR's deploy preview here (e.g., a specific component page).

Merge Checklist

  • If your update includes visual changes, have they been reviewed by a designer? Send them a link to the Netlify deploy preview, if applicable.
  • Does your change adhere to the documented style conventions?
  • Do any prop types have missing descriptions in the Props API tables in the documentation site (check deploy preview)?
  • Were your changes tested using all available themes (see theme switcher in the header of the deploy preview, under the "Settings" icon)?
  • Were your changes tested in the example app?
  • Is there adequate test coverage for your changes?
  • Consider whether this change needs to reviewed/QA'ed for accessibility (a11y). If so, please add wittjeff and adamstankiewicz as reviewers on this PR.

Post-merge Checklist

  • Verify your changes were released to NPM at the expected version.
  • If you'd like, share your contribution in #show-and-tell.
  • 🎉 🙌 Celebrate! Thanks for your contribution.

@openedx-webhooks openedx-webhooks added the blended PR is managed through 2U's blended developmnt program label Sep 22, 2023
@openedx-webhooks
Copy link

Thanks for the pull request, @khudym!

When this pull request is ready, tag your edX technical lead.

@netlify
Copy link

netlify bot commented Sep 22, 2023

Deploy Preview for paragon-openedx ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 6cdcca3
🔍 Latest deploy log https://app.netlify.com/sites/paragon-openedx/deploys/650d9c2a5b22bd00072678b4
😎 Deploy Preview https://deploy-preview-2646--paragon-openedx.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@codecov
Copy link

codecov bot commented Sep 22, 2023

Codecov Report

Patch and project coverage have no change.

Comparison is base (a06c3f3) 91.81% compared to head (6cdcca3) 91.81%.
Report is 6 commits behind head on master.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2646   +/-   ##
=======================================
  Coverage   91.81%   91.81%           
=======================================
  Files         235      235           
  Lines        4217     4217           
  Branches     1020     1020           
=======================================
  Hits         3872     3872           
  Misses        341      341           
  Partials        4        4           

see 1 file with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@khudym khudym force-pushed the fix-slider-colorpicker branch from ae9928f to 6cdcca3 Compare September 22, 2023 13:52
@brian-smith-tcril
Copy link
Contributor

I was curious about this so I did a little bit of digging. It seems the issue is that the react-colorful hex picker doesn't support invalid hex strings at all. After cloning the react-colorful repo and running npm run start-demo locally I looked at their example and found they get around this by simply not allowing invalid hex in the textbox at all

badcolor

another thing I tried was changing

<HexColorPicker color={color || ''} onChange={setColor} />

to be

<HexColorPicker color={hexValid ? color : '#fff'} onChange={setColor} />

which fixes the #NaNNaNNaN problem, but still feels less than ideal

nonan

I looked into the event handling for onChange, and realized it wasn't being fired when moving the hue slider (if the currently selected color doesn't have a hue, so anything on the far left of the picker box). I then found that this is a known issue that has been reported on their repo omgovich/react-colorful#206

I then tried another workaround (which feels a little hacky), instead using

<HexColorPicker color={hexValid ? color : '#fefefe'} onChange={setColor} />

fefefe

and that seems to give us expected behavior at least.

I'm not sure what the best path forward for this is. But at least it seems we have a few options to consider now!

@cintnguyen
Copy link
Contributor

@brian-smith-tcril

<HexColorPicker color={hexValid ? color : '#fefefe'} onChange={setColor} />

I think this is a good workaround for now to ensure there's always a hue to default to. Any idea why #fefefe turns into #fff?

@brian-smith-tcril
Copy link
Contributor

Any idea why #fefefe turns into #fff?

Good question!

tl:dr - rounding inaccuracy

full explanation below:

The hex value entered is parsed using hexToHsva.

export const hexToHsva = (hex: string): HsvaColor => rgbaToHsva(hexToRgba(hex));

which is calling hexToRgba

export const hexToRgba = (hex: string): RgbaColor => {
  if (hex[0] === "#") hex = hex.substring(1);

  if (hex.length < 6) {
    return {
      r: parseInt(hex[0] + hex[0], 16),
      g: parseInt(hex[1] + hex[1], 16),
      b: parseInt(hex[2] + hex[2], 16),
      a: hex.length === 4 ? round(parseInt(hex[3] + hex[3], 16) / 255, 2) : 1,
    };
  }

  return {
    r: parseInt(hex.substring(0, 2), 16),
    g: parseInt(hex.substring(2, 4), 16),
    b: parseInt(hex.substring(4, 6), 16),
    a: hex.length === 8 ? round(parseInt(hex.substring(6, 8), 16) / 255, 2) : 1,
  };
};

with #fefefe, hexToRgba is returning

{
  r: 254,
  g: 254,
  b: 254,
  a: 1,
}

which then goes into rgbaToHsva

export const rgbaToHsva = ({ r, g, b, a }: RgbaColor): HsvaColor => {
  const max = Math.max(r, g, b);
  const delta = max - Math.min(r, g, b);

  // prettier-ignore
  const hh = delta
    ? max === r
      ? (g - b) / delta
      : max === g
        ? 2 + (b - r) / delta
        : 4 + (r - g) / delta
    : 0;

  return {
    h: round(60 * (hh < 0 ? hh + 6 : hh)),
    s: round(max ? (delta / max) * 100 : 0),
    v: round((max / 255) * 100),
    a,
  };
};

which returns

{
  h: 0,
  s: 0,
  v: 100,
  a: 1,
}

which is then stored as the internal color value for the picker.

when we move the slider, react-colorful uses the color manipulation handleChange

  const handleChange = useCallback((params: Partial<HsvaColor>) => {
    updateHsva((current) => Object.assign({}, current, params));
  }, []);

which only modifies the h in the hsva object. the new internal value is then converted to hex here

fromHsva: ({ h, s, v }) => hsvaToHex({ h, s, v, a: 1 }),

which uses hsvaToHex from convert.ts

export const hsvaToHex = (hsva: HsvaColor): string => rgbaToHex(hsvaToRgba(hsva));

which calls hsvaToRgba

export const hsvaToRgba = ({ h, s, v, a }: HsvaColor): RgbaColor => {
  h = (h / 360) * 6;
  s = s / 100;
  v = v / 100;

  const hh = Math.floor(h),
    b = v * (1 - s),
    c = v * (1 - (h - hh) * s),
    d = v * (1 - (1 - h + hh) * s),
    module = hh % 6;

  return {
    r: round([v, c, b, b, d, v][module] * 255),
    g: round([d, v, v, c, b, b][module] * 255),
    b: round([b, b, d, v, v, c][module] * 255),
    a: round(a, 2),
  };
};

which, for {h: ANYTHING, s: 0, v: 100, a: 1}, returns

{
  r: 255,
  g: 255,
  b: 255,
  a: 1
}

which, if we don't pay attention to a, is #ffffff

@openedx-webhooks
Copy link

@khudym Even though your pull request wasn’t merged, please take a moment to answer a two question survey so we can improve your experience in the future.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blended PR is managed through 2U's blended developmnt program
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants