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

Replaced the regex-based transformation of CSS to XPath #52

Merged
merged 1 commit into from
Jul 18, 2014

Conversation

stof
Copy link
Collaborator

@stof stof commented May 28, 2014

The library now uses the Symfony CssSelector component, which supports more cases than the custom logic used previously.
Closes #34

I updated the doc to mention composer as the recommended installation way, as this takes care of including the CssSelector component automatically.
Btw, don't forget to update the composer.json again when bumping the version after this change :)

The library now uses the Symfony CssSelector component, which supports
more cases than the custom logic used previously.
Closes tijsverkoyen#34
@hacfi
Copy link

hacfi commented Jun 10, 2014

👍 Much simpler

barryvdh added a commit to barryvdh/CssToInlineStyles that referenced this pull request Jul 16, 2014
This searches all style tags for Media Queries, and replaces the content with just those. Fixes tijsverkoyen#45

Also set the default to strip mediaqueries from inline tags, because that doesn't work as expected. (at least, not after tijsverkoyen#52)

Not sure if this is the most performant/optimal way, so open for ideas ;)
@barryvdh barryvdh mentioned this pull request Jul 16, 2014
@barryvdh
Copy link
Contributor

This does seem to have some side-effects when using mediaqueries, probably because it handles them more strict?
Setting setExcludeMediaQueries(true) makes it behave as earlier :)
I tweaked the default behaviour for this, together with my PR to preserve mediaqueries in #54

@stof
Copy link
Collaborator Author

stof commented Jul 16, 2014

@barryvdh what is the issue you face with media queries ?

@barryvdh
Copy link
Contributor

This example html: http://zurb.com/ink/downloads/templates/sidebar-hero.html

This php:

$inliner = new TijsVerkoyen\CssToInlineStyles\CssToInlineStyles($html);
$inliner->setStripOriginalStyleTags(true);
$inliner->setUseInlineStylesBlock(true);
$stripped = $inliner->convert();

Results in 2 different outputs: http://i.imgur.com/f6Y7d1C.png

Adding $inliner->setExcludeMediaQueries(true); makes it behave the same as before.
Not sure why you would want to include the mediaqueries, so not problem excluding it, but it's a bit strange. But probably because the old xpath didn't handle mediaqueries properly?

@stof
Copy link
Collaborator Author

stof commented Jul 16, 2014

anyway, excluding media queries is the sensible default when inlining, as media queries cannot get inlined.

but the difference is strange indeed. This make me think that some of the rules may have been transformed to the wrong XPath previously, and so not matching.

@barryvdh
Copy link
Contributor

A think to note is that does hurt performance a bit. In the referenced template, converting 10 templates go from 550ms to 1100 ms, so it's about twice as slow in some cases. Not sure if there is anything we can do to speed it up?

@barryvdh
Copy link
Contributor

The Zend class mentioned in the original issue, seems to bit a bit faster (when just copying the tokenize/csstoxpath function): https://github.com/zendframework/zf2/blob/master/library/Zend/Dom/Document/Query.php, around 700 ms with the same template. Not sure if that's less good or misses some cases.

@stof
Copy link
Collaborator Author

stof commented Jul 16, 2014

well, the Zend class is also based on regex transformations, which will handle less cases (and produce broken XPath queries for cases they don't support, leading to error messages which are impossible to understand)

@barryvdh
Copy link
Contributor

Okay cool, as long as it's better I'm fine with losing some performance :)

@stof
Copy link
Collaborator Author

stof commented Jul 16, 2014

note that we may improve performance in future versions of Symfony (btw, if you can run your CssSelector benchmark with xhprof to identify the bottleneck, it might even happen faster).
The latest version of Symfony already improved the memory usage (letting PHP recover memory more easily at the end of the conversion by avoiding cyclic references).

@tijsverkoyen what do you think about this PR ?

@tijsverkoyen
Copy link
Owner

Hi @stof, first of all: big thanks. I will review the code soon.

@tijsverkoyen tijsverkoyen merged commit 3ea5e0e into tijsverkoyen:master Jul 18, 2014
@tijsverkoyen
Copy link
Owner

Merged, thx @stof!

@stof stof deleted the use_symfony_css_selector branch July 18, 2014 13:43
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.

buildXPathQuery doesn't properly handle multiple classes
4 participants