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 floating point hue in hsl() and hsla() #31

Merged
merged 3 commits into from
Dec 3, 2024

Conversation

erxclau
Copy link
Contributor

@erxclau erxclau commented Nov 30, 2024

Browsers support a color like hsl(120.152, 100%, 75%) but this library would incorrectly parse the hue and fallback to black.

Changes parsing the hue as an integer to parsing as a number.

CSS Color Module Level 3 4.2.4 suggests that the hue should be a number (and not strictly an integer):

Hue is represented as an angle of the color circle (i.e. the rainbow represented in a circle). This angle is so typically measured in degrees that the unit is implicit in CSS; syntactically, only a <number> is given.

CSS Color Module Level 4 is more explicit that hue can be a number.

This PR also comments out the parse_integer and parse_list_integer functions (and relevant tests) from src/stream.rs because leaving them in the code produced these warnings during build:

warning: methods `parse_integer` and `parse_list_integer` are never used
   --> src/stream.rs:417:12
    |
109 | impl<'a> Stream<'a> {
    | ------------------- methods in this implementation
...
417 |     pub fn parse_integer(&mut self) -> Result<i32, Error> {
    |            ^^^^^^^^^^^^^
...
447 |     pub fn parse_list_integer(&mut self) -> Result<i32, Error> {
    |            ^^^^^^^^^^^^^^^^^^
    |
    = note: `#[warn(dead_code)]` on by default

Happy to uncomment or remove them completely if that's preferred.

Related to vercel/satori#602

@erxclau erxclau changed the title feat: support floating point hue in hsl() and hsla() Support floating point hue in hsl() and hsla() Nov 30, 2024
@DJMcNab
Copy link
Member

DJMcNab commented Dec 2, 2024

Thanks, this seems reasonable!
I'd strongly suggest that we shouldn't land this with the dead code just commented out. I'd lean towards just removing it entirely.

@erxclau
Copy link
Contributor Author

erxclau commented Dec 3, 2024

@DJMcNab I removed the dead code

@DJMcNab DJMcNab added this pull request to the merge queue Dec 3, 2024
Merged via the queue into linebender:main with commit 8fdef18 Dec 3, 2024
15 checks passed
@waywardmonkeys waywardmonkeys added this to the 0.15.3 milestone Dec 28, 2024
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.

3 participants