[BUGFIX] Render RGB functions with "modern" syntax if required #840
Conversation
JakeQZ
added this to the
9.0.0: Drop support for PHP < 7.2 and clean up things milestone
|
This is part-bug, part-feature. It could be viewed as a bug in the new feature, or just a continuation of the implementation of that feature. |
The "legacy" syntax does not allow a mixture of `percentage`s and `number`s for the red, green and blue components. So if `rgb`/`rgba` functions that have such a mixture are rendered in the "legacy" syntax, an invalid property value will result. An `OutputFormat` option to use the "modern" syntax throughout will be added later (see #801).
JakeQZ
force-pushed
the
feature/color/render-modern
branch
from
72df316 to
e70a55f
Compare
oliverklee
left a comment
oliverklee
left a comment
There was a problem hiding this comment.
Looks functionally okay to me. I think we can make the code more readable and easier to grok, though. I've left some comments.
src/Value/Color.php
Outdated
| private function renderInModernSyntax(OutputFormat $outputFormat): string | ||
| { | ||
| \end($this->aComponents); | ||
| if (\key($this->aComponents) === 'a') { |
There was a problem hiding this comment.
I'd prefer if we don't rely on $this->aComponents having the associative array keys being in a any particular order. (This also is nothing which we currently can annotate and hence check with PHPStan.)
Instead, we can use array_key_exists or isset.
There was a problem hiding this comment.
The order is specific and comes from the CSS function name (e.g. rgba). There's a newer one lab in which a is not alpha. a is only alpha if it is the final element (in all cases I'm aware of).
src/Value/Color.php
Outdated
| $alpha = $this->aComponents['a']; | ||
| $componentsWithoutAlpha = \array_diff_key($this->aComponents, ['a' => 0]); | ||
| } else { | ||
| $componentsWithoutAlpha = $this->aComponents; |
There was a problem hiding this comment.
If we use unset, we also don't need the if/else.
There was a problem hiding this comment.
Except if we want
a local variable
$hasAlpha
we might still need an else clause.
Co-authored-by: Oliver Klee <[email protected]>
JakeQZ
force-pushed
the
feature/color/render-modern
branch
from
ae35316 to
148f265
Compare
|
I've made most suggested changes, and commented on some as to why they can't be made. There's a remaining dichotomy (I can only make or other of two suggested changes). Hopfully this will be clearer now, with the other changes made... |
References to add to final commit message: - https://developer.mozilla.org/en-US/docs/Web/CSS/color_value/rgb#formal_syntax - https://www.w3.org/TR/css-color-4/#rgb-functions
JakeQZ
force-pushed
the
feature/color/render-modern
branch
from
84421d4 to
e966c06
Compare
|
I added some spec links to the latest commit message (re "legacy" and "modern") which will need to be integrated into the final commit message somehow, if desired. |
|
I've addressed most issues, but there are still two unresolved that require your feedback (see the comments). |
|
By the way, I see this in the commit messages of your PRs: Are you using two different email addresses? And are both registered in your GitHub account? |
I've edited some PR commit messages to have this email address, because I don't want my main one listed on GitHub. I'm getting increasing amounts of spam arising from email addresses being scraped from GitHub. Ideally I would change my GitHib account to use this address, but I suspect that would be some hassle. |
|
Have you tried adding it as an additional email address? |
Didn't know that was possible. Thanks. |
The "legacy" syntax does not allow a mixture of
percentages andnumbers for the red, green and blue components.So if
rgb/rgbafunctions that have such a mixture are rendered in the "legacy" syntax, an invalid property value will result.An
OutputFormatoption to use the "modern" syntax throughout will be added later (see #801).