-
Notifications
You must be signed in to change notification settings - Fork 148
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
Fix/css4 rgb parsing #202
base: main
Are you sure you want to change the base?
Fix/css4 rgb parsing #202
Conversation
+1 for this PR, encountered the issue today! |
Bump |
@@ -26,3 +26,8 @@ | |||
background-color: rgba(var(--some-rg), 255, 0.1); | |||
background-color: hsla(var(--some-hsl), 0.1); | |||
} | |||
|
|||
#css4-rgba { | |||
background-color: rgb(242 245 249 / 45%); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be cool to have an output format option to prefer this syntax over the other. AFAICT there are three boolean options that are all independent of one another: prefer-rgb-over-rgba / rgb-alpha-separator-use-slash / rgb-alpha-use-percentage.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would, though can (and should) be done as a separate PR.
src/Value/Color.php
Outdated
@@ -66,14 +66,21 @@ public static function parse(ParserState $oParserState) | |||
$oParserState->consume('('); | |||
|
|||
$bContainsVar = false; | |||
$iLength = $oParserState->strlen($sColorMode); | |||
if (strpos($sColorMode, 'rgb') !== false) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rgb
has to be at the start, right?
if (strpos($sColorMode, 'rgb') !== false) { | |
if (str_starts_with($sColorMode, 'rgb')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Though I prefer being explicit:
if (strpos($sColorMode, 'rgb') !== false) { | |
if ($sColorMode === 'rgb') { |
(the rgba
case will be handled in the else
branch anyway).
Hi, I've updated the PR to include the recommended stricter checks for |
Hi, I am using this library for the first time in https://www.drupal.org/project/ui_styles/issues/3453784. I am encountering the same issue has #357. And I confirm that the changes in this PR are fixing the bug. Thanks! What remains to be done to have it merged and shipped in a new release? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code changes look good to me. However, it would be good to have a few more tests:
- to cover the various separators (space, comma, slash);
- to cover
hsl
with alpha.
Also, could you add an entry to CHANGELOG.md
(newest first).
In CSS Level 4
rgba()
andhsla()
are aliases forrgb()
andhsl()
which now accept the alpha argument. This PR reflects this behavior and makes the parser understand the syntax. Source: https://developer.mozilla.org/en-US/docs/Web/CSS/color_value#rgb()_and_rgba()Example: