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

addExcludedSelector()/removeExcludedSelector() #207

Closed
wants to merge 1 commit into from

Conversation

vanderlee
Copy link
Contributor

Added methods addExcludedSelector() and removeExcludedSelector() that let you add selectors which should be excluded from processing. This is convenient to filter out nodes that are needed by other tools without having to change the style.

A unittest is included.

@vanderlee
Copy link
Contributor Author

Whenever I squash this and try to push, I just get forced into an endless loop of pull+merge upstream branch and having to redo the squash again.

Should I push the squashed feature in a different branch?

@vanderlee vanderlee force-pushed the exclude-selectors branch from 8fed3c8 to 6e950de Compare July 11, 2015 18:25
@vanderlee
Copy link
Contributor Author

Never mind; got it. Needed to do a force push.
This also seems to retroactively change this thread.

@@ -72,6 +72,8 @@ class Emogrifier
* @var string
*/
private $css = '';

private $excludedSelectors = [];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add an @ var PHPDoc.

@oliverklee
Copy link
Contributor

Please also add a few lines about this new feature to the README.

{
$css = 'p { margin: 0; }';
$this->subject->setHtml($this->html5DocumentType . '<html><body>' .
'<p class="x"></p><p></p></body></html>');
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I can see, the second p tag is not important for this test.

@vanderlee
Copy link
Contributor Author

I'm done.

This, including all the tweaks, could have been done many hours of work ago and I no longer feel my time is best spent continuing on.

Feel free to take over this and my other changes.

@oliverklee
Copy link
Contributor

@vanderlee That's very sad to hear. I really appreciate your contributions.

@oliverklee
Copy link
Contributor

Hi @vanderlee, I've polished your changes, created a new PR #215 for them and merged the new PR. Thanks for contributing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants