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

Support all CSS color formats #12

Closed
15 tasks done
sharkdp opened this issue Jun 16, 2019 · 7 comments
Closed
15 tasks done

Support all CSS color formats #12

sharkdp opened this issue Jun 16, 2019 · 7 comments
Labels
enhancement New feature or request good first issue Good for newcomers

Comments

@sharkdp
Copy link
Owner

sharkdp commented Jun 16, 2019

See https://developer.mozilla.org/en-US/docs/Web/CSS/color_value for all details and test cases

  • hexadecimal RRGGBB (#ff0000)
  • hexadecimal RGB (#f00)
  • rgb(0, 0, 255)
  • rgb(0%, 50%, 100%)
  • hsl(120, 100%, 75%)
  • hsl(…,…,…) in all variations
    • With and without ° sign
    • Negative angles
  • predefined color names
  • CSS Level 4 predefined names (rebeccapurple)

optional:

  • CSS Color Level 4: space-separated rgb(R G B[ A]) - only a draft
  • gray(0.3)
  • gray(30%)
  • rad instead of degrees
  • turns instead of degrees
@sharkdp sharkdp modified the milestone: Initial release Jun 16, 2019
@sharkdp sharkdp added this to the Initial release milestone Aug 1, 2019
sharkdp added a commit that referenced this issue Aug 1, 2019
@sharkdp sharkdp removed this from the Initial release milestone Aug 24, 2019
@sharkdp sharkdp added enhancement New feature or request good first issue Good for newcomers labels Aug 29, 2019
@saidsay-so
Copy link
Contributor

I implemented most of the functions here. Do I make one PR for all or a PR for each commit ?

@sharkdp
Copy link
Owner Author

sharkdp commented Oct 6, 2019

One for all sounds great. Thank you very much.

@saidsay-so
Copy link
Contributor

No problem!

@sharkdp
Copy link
Owner Author

sharkdp commented Oct 8, 2019

After the updates by @musikid, we do now support almost every possible color format in the CSS spec.

While testing the examples given on the website mentioned above, I found two cases which were not supported yet:

  • rgb(255, 0, 153.0) - mixed integer and float RGB values
  • hsl(270 60% 70%) - hsl(…) with whitespace separation

Maybe we should try to solve these in a more general case (instead of adding even more regex patterns):

  • allow whitespace separation everywhere(?) where comma-separated values are now accepted (hsl, lab, …)
  • allow for floating point values in some cases where we restrict to integer values right now

There were a few more cases which also did not work, but I don't think we have to necessarily support them (mostly involving alpha values).

@halfbro
Copy link

halfbro commented Oct 13, 2019

Hey, I'm interested in tackling the general parsing problem here. Do you think that using a parser combinator library (such as nom) would be a good solution here?

@sharkdp
Copy link
Owner Author

sharkdp commented Oct 13, 2019

Hey, I'm interested in tackling the general parsing problem here

fantastic 👍

Do you think that using a parser combinator library (such as nom) would be a good solution here?

That's what I was thinking about when writing "… instead of adding even more regex patterns" above 😄.

I initially wondered if I should use a parser combinator library when writing the first few patterns, but I wasn't sure if it would definitely be the better solution. We certainly don't need all the power of a generic parser here. It's definitely manageable with a collection of regex patterns, but the code duplication isn't exactly great.

In short, I'd be in favor of a parser-combinator solution if:

  • it would be easier to understand and maintain than the regex solution
  • it would not be much slower (it's probably much faster, but that should be checked)
  • it's lightweight (doesn't add a hueg amount of dependencies)

@halfbro
Copy link

halfbro commented Oct 14, 2019

I started this last night, I got this far, using nom:

fn opt_hash_char(s: &str) -> IResult<&str, Option<char>> {
    opt(char('#'))(s)
}

fn parse_hex(input: &str) -> IResult<&str, Color> {
    let (input, _) = opt_hash_char(input)?;
    let (input, hex_chars) = hex_digit1(input)?;
    match hex_chars.len() {
        6 => {
            let r = hex_to_u8_unsafe(&hex_chars[0..2]);
            let g = hex_to_u8_unsafe(&hex_chars[2..4]);
            let b = hex_to_u8_unsafe(&hex_chars[4..6]);
            Ok((input, rgb(r, g, b)))
        }
        3 => {
            let r = hex_to_u8_unsafe(&hex_chars[0..1]);
            let g = hex_to_u8_unsafe(&hex_chars[1..2]);
            let b = hex_to_u8_unsafe(&hex_chars[2..3]);
            let r = r * 16 + r;
            let g = g * 16 + g;
            let b = b * 16 + b;
            Ok((input, rgb(r, g, b)))
        }
        _ => Err(Err::Error((
            "Expected hex string of 3 or 6 characters length",
            nom::error::ErrorKind::Many1,
        ))),
    }
}

Which handles all the hex rgb cases, AFAICT. A lot simpler than 2-3 regexes, imo. Using nom also allows us to get error messages about the input, if that seems useful.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request good first issue Good for newcomers
Projects
None yet
Development

No branches or pull requests

3 participants