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

parseToHsl Doesn't seem to parse valid CSS Colors 4 space-separated values #605

Closed
josh-degraw opened this issue Mar 20, 2022 · 9 comments · Fixed by #606
Closed

parseToHsl Doesn't seem to parse valid CSS Colors 4 space-separated values #605

josh-degraw opened this issue Mar 20, 2022 · 9 comments · Fixed by #606
Labels

Comments

@josh-degraw
Copy link

josh-degraw commented Mar 20, 2022

  • polished version: 4.1.3
  • Any relevant JS library and version: @storybook/react v6.4.19

What You Are Seeing

Parsing values such as 'hsl(235 100% 50%)' or 'hsl(246deg 37% 15% / 100%)' throws an error saying: Couldn't parse the color string. Please provide the color as a string in hex, rgb, rgba, hsl or hsla notation.

I'm just trying to run storybook and integrate new brand colors by our design team, which are written in as hsl values in space-separated format. Attempting to use certain values in the storybook theme throw an error. I'm unsure where exactly this is used in storybook's lib, as the error doesn't seem to get thrown on some places (e.g. MDX stories), but I was able to reproduce the error by calling the parseToHsl function manually. I believe I can hardcode a different syntax in the storybook theme as a temporary workaround, but I would expect this syntax to be supported here.

What You Expected To See

No error should be thrown, because that is valid syntax supported by most browsers according to MDN here and here.

Reproduction

Just try calling parseToHsl('hsl(235 100% 50%)') or parseToHsl('hsl(246deg 37% 15% / 100%)')

EDIT: Sandbox link: https://codesandbox.io/s/focused-agnesi-mw1t4r?file=/src/App.js

@taitems
Copy link

taitems commented Mar 31, 2022

I was able to write a temporary workaround (hack) to ensure my input to the function is transformed with the parse-css-color package

import parse from 'parse-css-color';

const backportHsl = str => {
  const { values } = parse(str);
  return `hsl(${values[0]}, ${values[1]}%, ${values[2]}%)`;
};

And although this is untested code you could probably handle both rgba and hsla with something loosely like the below:

// unverified idea
const backport = str => {
  const { type, values, alpha } = parse(str);
  const suffix = type === 'hsl' ? '%' : '';
  return `${type}a(${values[0]}, ${values[1] + suffix}, ${values[2] + suffix}, ${alpha})`;
};

@josh-degraw
Copy link
Author

Yeah, I ended up doing something similar that's worked for me as a temporary workaround:

const hslReg = /hsl\((?<h>\d+)deg\s*(?<s>[\d.]+)%\s*(?<l>[\d.]+)%\s*\/\s*(?<a>\d+)%\)/
const designTokens = Object.entries(originalTokens).reduce(
  (total, [key, value]) => {
    const match = hslReg.exec(value)
    if (match) {
      const {h, s, l, a} = match.groups ?? {}
      const hsla = `hsla(${h}, ${s}%, ${l}%, ${a}%)`
      total[key] = hsla
    } else {
      total[key] = value
    }

    return total
  },
  {} 
) 

@bhough bhough added bug and removed unconfirmed bug labels Apr 1, 2022
@bhough
Copy link
Contributor

bhough commented Apr 1, 2022

@josh-degraw Thank you for raising this. I've verified the bug and am working on a fix.

@taitems
Copy link

taitems commented Apr 4, 2022

Hey @bhough, can I please get this re-opened?

Looking at your tests it appears that you solved for hsl sans alpha, and hsla with alpha, but looking at the colour spec you can provide an optional alpha value to hsl.

The following value is what was breaking both before and after the release:

hsl(156deg 99% 29% / 100%)

@bhough
Copy link
Contributor

bhough commented Apr 4, 2022

@taitems The issue is the use of a percentage in the opacity, this should work if you use a float. Will cut a new release shortly.

@taitems
Copy link

taitems commented Apr 4, 2022

Thank you kindly @bhough!

@bhough
Copy link
Contributor

bhough commented Apr 4, 2022

Version 4.2.1, should now hopefully fully address this.

@josh-degraw
Copy link
Author

Thank you for getting this fixed!

@taitems
Copy link

taitems commented Apr 4, 2022

Can confirm this is working, thanks again.

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

Successfully merging a pull request may close this issue.

3 participants