[TASK] Separate CssInliner::inlineCss and the rendering #654
Conversation
oliverklee
force-pushed
the
task/inline-css
branch
from
f3e262c
to
c080a14
Compare
src/Emogrifier/CssInliner.php
Outdated
| $this->css = $css; | ||
| $this->process($css); | ||
|
|
||
| return $this; | ||
| } |
There was a problem hiding this comment.
Could we dispense with this method and instead rename process() to inlineCss() (and have it return $this for chaining)?
| @@ -399,7 +364,6 @@ protected function process() | |||
|
|
|||
| // grab any existing style blocks from the html and append them to the existing CSS | |||
| // (these blocks should be appended so as to have precedence over conflicting styles in the existing CSS) | |||
| $allCss = $this->css; | |||
| if ($this->isStyleBlocksParsingEnabled) { | |||
| $allCss .= $this->getCssFromAllStyleNodes($xPath); | |||
| } | |||
There was a problem hiding this comment.
This modifies the parameter passed to the function. Should the parameter be a separate entity (e.g. $css) with the preceding line being changed to $allCss = $css; rather than removed?
| @@ -430,11 +440,11 @@ public function wbrTagDataProvider() | |||
| * | |||
| * @dataProvider wbrTagDataProvider | |||
| */ | |||
| public function emogrifyByDefaultRemovesWbrTag($html) | |||
| public function renderByDefaultRemovesWbrTag($html) | |||
| { | |||
There was a problem hiding this comment.
Should the method name be inlineCss... rather than render..., since it's the inlineCss method that removes these elements?
|
|
||
| $result = $subject->emogrify(); | ||
| $result = $subject->render(); | ||
|
|
There was a problem hiding this comment.
Now that inlineCss() performs the action being tested, the blank lines separating 'set-up', 'execute' and 'check result' no longer achieve that logical grouping (this also applies to many other tests below).
|
Thanks for the review! I've updated the PR according to the review comments and repushed. |
oliverklee
force-pushed
the
task/inline-css
branch
from
c080a14
to
167b011
Compare
This is another step toward making `CssInliner` a `HtmlProcessor`.
oliverklee
force-pushed
the
task/inline-css
branch
from
167b011
to
cc60d45
Compare
This is another step toward making
CssInlineraHtmlProcessor.