Add support for CSS3 calc #128
Conversation
sabberworm
left a comment
There was a problem hiding this comment.
Thanks a lot for your work!
Could you take a look at the issues I raised?
lib/Sabberworm/CSS/Parser.php
Outdated
| } | ||
|
|
||
| private function parseCalcValue() { | ||
| $func = $this->comes('calc', true) ? 'calc' : '-webkit-calc'; |
There was a problem hiding this comment.
You could use $func = trim(consumeUntil('(')) to streamline this.
lib/Sabberworm/CSS/Parser.php
Outdated
| $oValue = $this->parseColorValue(); | ||
| } else if ($this->comes('url', true)) { | ||
| $oValue = $this->parseURLValue(); | ||
| } else if ($this->comes('calc', true) || $this->comes('-webkit-calc', true)) { |
There was a problem hiding this comment.
Support -moz-calc as well. Also, is there an -o-calc?
There was a problem hiding this comment.
Done for -moz-calc, but -o-calc does not seem to exist https://www.w3schools.com/cssref/func_calc.asp
lib/Sabberworm/CSS/Parser.php
Outdated
| $this->consume($func); | ||
| $this->consumeWhiteSpace(); | ||
| $this->consume('('); | ||
| $aArguments = $this->parseValue(array('+', '-', '*', '/', ' '), '\Sabberworm\CSS\Value\CalcRuleValueList'); |
There was a problem hiding this comment.
Parsing the arguments using parseValue probably means an expression like calc(50% -8px) would be treated as calc(50% - 8px), not as an error (like it should).
There was a problem hiding this comment.
Am I right in assuming that this does not support parenthesized subexpressions like calc( ( 100px / 2) / 2)?
There was a problem hiding this comment.
Correct for both. Should be fixed now :)
lib/Sabberworm/CSS/OutputFormat.php
Outdated
| public $sSpaceAfterListArgumentSeparator = ''; | ||
|
|
||
| // This is what’s printed after the operators in a calc function | ||
| public $sSpaceBeforeCalcListArgumentSeparator = ' '; |
There was a problem hiding this comment.
Since the spaces around - and + operators are mandatory, their output should not be affected by OutputFormat.
There was a problem hiding this comment.
Yes, I removed this entirely in the last commit.
Fixes issue #79