-
Notifications
You must be signed in to change notification settings - Fork 154
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
[FEATURE] Add isRemoveDisplayNoneEnabled option + tests #162
[FEATURE] Add isRemoveDisplayNoneEnabled option + tests #162
Conversation
b706250
to
f2d81ad
Compare
* Determines whether elements with the `display: none` property are | ||
* removed from the DOM. | ||
*/ | ||
private $isRemoveDisplayNoneEnabled = true; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, the option name is a bit misleading. As it is about removing/keeping nodes that are display: none (and not about removing/keeping the display: none itself), I propose the name "shouldKeepInvisibleNodes". What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, that sounds fine.
cf1a367
to
5dd7e05
Compare
@oliverklee I think i've made all the requested changes. Thanks for the feedback! |
@@ -338,6 +331,16 @@ public function disableStyleBlocksParsing() | |||
} | |||
|
|||
/** | |||
* Disables the removal of elements with `display: none` properties |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please add the period at the end of the sentence.
Could you also please write a short documentation for this new feature for the README? |
5dd7e05
to
569fdcd
Compare
Made the requested changes. The test failure looks Github related? Travis couldn't connect to github. I'm not sure how to requeue the test - @oliverklee could you do that? |
I've run the tests on this branch locally (as long as Travis does not play along). On your branch, I get the following failure:
Do the tests run fine on your local machine? |
And could you please prefix the commit message subject line with [FEATURE]? |
$nodesWithStyleDisplayNone = $xpath->query( | ||
'//*[contains(translate(translate(@style," ",""),"NOE","noe"),"display:none")]' | ||
); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove this blank line so that the guard clause is a single visible block.
This adds an options parameter and some tests for a `removeDisplayNone` option which specifies if the Emogrifier parser will remove elements with the style "display: none". Unfortunatly, the nice little optimization has some consiquences. When writing responsive email templates, "display: none" is used often to hide certain sections of the template for desktop email clients and show them in phone clients. I can see how it'd be nice to clean up elements that will not be displayed in some cases, so that's why I made it an options field.
569fdcd
to
b32411f
Compare
I address all your concerns - still looks like Travis isn't working with github. |
I've tested the PR locally, fixed the unit tests (the DIVs were not nested, which made the assertContains not find any potential problems reliably), merged locally, and pushed. Thanks for contributing, @timothyasp! ❤️ |
This adds an options parameter and some tests for a
isRemoveDisplayNoneEnabled
optionwhich specifies if the Emogrifier parser will remove elements with the style
"display: none".
Unfortunatly, the nice little optimization has some consiquences.
When writing responsive email templates, "display: none" is used often to hide
certain sections of the template for desktop email clients and show them in
phone clients.
I can see how it'd be nice to clean up elements that will not be displayed in
some cases, so that's why I made it an options field.