Add style blocks to body rather than head v2 #396

Renkas · 2017-07-29T12:42:49Z

This is modification of existing pull request: #359

I removed the option to move style element to body and made it default/only way.
Added unit test and modified some existing ones.

Issue356

This reverts commit 4edd60d, reversing changes made to b0d9f40.

oliverklee

I've added some review comments.

oliverklee · 2017-07-31T16:43:04Z

CHANGELOG.md

@@ -9,7 +9,8 @@ This project adheres to [Semantic Versioning](http://semver.org/).
 ### Added
 - Debug mode. Throw debug exceptions only if debug is active.
  ([#392](https://github.com/jjriv/emogrifier/pull/392))
-
+- Added setting for putting the media query style block to <body> vs <head>
+  ([#396](https://github.com/jjriv/emogrifier/pull/396))


The entries should be ordered in reverse chronological order, i.e., newest on top. Could you please move this entry to the top of the "Added" section?

oliverklee · 2017-07-31T16:46:00Z

Classes/Emogrifier.php

@@ -1037,8 +1037,10 @@ protected function addStyleElementToDocument(\DOMDocument $document, $css)
        $styleAttribute->value = 'text/css';
        $styleElement->appendChild($styleAttribute);

-        $head = $this->getOrCreateHeadElement($document);


The method getOrCreateHeadElement then is not needed anymore and can be removed.

It is removed

oliverklee · 2017-07-31T17:04:38Z

Classes/Emogrifier.php

+        $body = $this->getBodyElement($document);
+        if ($body) {
+            $body->appendChild($styleElement);
+        }


This means that the style tag will not be added if there is no body element, while the body element actually is optional in HTML. So we have several options (orderd by how much I like them):

If there is no BODY element present, we create one and move all children of the HTML into it (except for the HEAD element if it is present).

If there is no BODY element present, we attach the style element directly to the HTML element.

If there is no body element, we throw an exception.

What do you think?

(This behavior also needs to be covered by unit tests.)

Only way there would not be a body tag is if there is no content in document. In all other cases DOMDocument will add the body tag automatically. But document without content is useless anyway.

Are you sure?

As far as I know, the HEAD and BODY elements are optional, i.e., the child tags (TITLE etc. for HEAD, H1 etc. for BODY) can be direct children of the HTML element. Does DOMDocument then automatically sort the existing tags correctly in either HEAD or BODY, or will e.g., TITLE also go into BODY?

oliverklee · 2017-07-31T17:05:50Z

Classes/Emogrifier.php

+     *
+     * @param \DOMDocument $document
+     *
+     * @return \DOMNode the body element


This must be \DOMNode|null because the return value will be null if there is no BODY element.

oliverklee · 2017-07-31T17:06:26Z

Classes/Emogrifier.php

-        $head = $this->getOrCreateHeadElement($document);
-        $head->appendChild($styleElement);
+        $body = $this->getBodyElement($document);
+        if ($body) {


$body isn't a bool, but either a \DOMNode instance or null. So the check needs to be !== null.

oliverklee · 2017-07-31T17:08:35Z

Tests/Unit/EmogrifierTest.php

@@ -1111,7 +1111,7 @@ public function emogrifyKeepExistingHeadElementContent()
     */
    public function emogrifyKeepExistingHeadElementAddStyleElement()


The name of this tests does not make sense anymore with the new behavior.

oliverklee · 2017-07-31T17:08:51Z

Tests/Unit/EmogrifierTest.php

@@ -1121,6 +1121,23 @@ public function emogrifyKeepExistingHeadElementAddStyleElement()
    }

    /**
+     * @test
+     */
+    public function emogrifyMoveStyleElementFromHeadToBody()


Third person, please: Move -> Moves

oliverklee · 2017-07-31T17:09:43Z

Tests/Unit/EmogrifierTest.php

+        $result = $this->subject->emogrify();
+
+        self::assertContains(
+            '<body><style type="text/css">@media all { html {  color: red; } }</style></body>',


Please let's move the STYLE node into a local variabel to make this shorter and also communicate the intent more clearly.

oliverklee · 2017-08-24T10:48:27Z

@Renkas Any news on this PR? (Please don't feel discouraged by the review comments. I aim to get your changes into the code base in their best form.)

oliverklee · 2017-09-10T16:25:21Z

CHANGELOG.md

@@ -7,6 +7,8 @@ This project adheres to [Semantic Versioning](http://semver.org/).
 ## x.y.z (unreleased)

 ### Added
+- Added setting for putting the media query style block to <body> vs <head>


I propose we simplify/shorten this to: "Automatically the STYLE block into the BODY". What do you think=

oliverklee · 2017-09-17T12:29:28Z

We've moved the repository to https://github.com/MyIntervals/emogrifier . If you have an "upstream" branch in your fork, please make sure to update the URL:

git remote set-url upstream git@github.com:MyIntervals/emogrifier.git

Renkas · 2017-09-20T18:52:23Z

So what is the status?

oliverklee · 2017-09-20T19:08:53Z

I seems that you have re-pushed. Unfortunately. GitHub does not send any notifications of this. :-(

Thanks for the reminder! 🍪 I'll have another look at the PR.

This is a prerequisite to #396.

oliverklee · 2017-10-13T10:08:41Z

I've just created #410 for making sure we always have a BODY element.

This is a prerequisite to #396.

oliverklee · 2017-10-14T09:36:04Z

@Renkas Now that #410 is merged (which makes sure we always have a BODY element), could you please rebase and make use of the BODY element always being there? Thanks!

oliverklee · 2017-10-16T17:19:49Z

Merged. Thanks for contributing, @Renkas and @Gabe-SpotMe! ❤️

Gabe-IncentFit and others added 9 commits January 7, 2017 15:50

adding option to add style blocks to body rather than head

6e190fc

adding changelog and readme

9e254d2

Add support fo :not selector

f4a93af

Changelog entry

727bc50

Fix Undefined offset: 1

c6e120c

CS fixes

973c64f

Merge remote-tracking branch 'jjriv/master' into pr/1

b6432af

Add tests and fix some code

7b703e8

Change PR number

b0d9f40

oliverklee self-requested a review July 29, 2017 13:41

oliverklee assigned Renkas Jul 29, 2017

oliverklee added this to the 1.3.0 milestone Jul 29, 2017

Renkas and others added 3 commits July 30, 2017 20:26

Merge pull request #2 from Renkas/issue356

bf78639

Issue356

Merge branch 'master' into pr/1

4edd60d

Revert "Merge branch 'master' into pr/1"

d573f13

This reverts commit 4edd60d, reversing changes made to b0d9f40.

oliverklee added the in review label Jul 31, 2017

oliverklee requested changes Jul 31, 2017

View reviewed changes

oliverklee added the waiting for feedback label Aug 24, 2017

Renkas added 3 commits September 10, 2017 15:13

Merge remote-tracking branch 'jjriv/master' into pr/1

6ceba01

Reorder changelog entry

2fa056d

Changes

c799294

oliverklee mentioned this pull request Sep 10, 2017

Add style blocks to body rather than head #359

Closed

oliverklee reviewed Sep 10, 2017

View reviewed changes

Renkas and others added 2 commits September 10, 2017 21:44

Rephrase changelog entry

fc7f9c3

Merge branch 'master' into pr/1

beebc4f

oliverklee removed the waiting for feedback label Oct 13, 2017

oliverklee added a commit that referenced this pull request Oct 13, 2017

[BUGFIX] Make sure the HTML always has a BODY element

808b232

This is a prerequisite to #396.

oliverklee mentioned this pull request Oct 13, 2017

[BUGFIX] Make sure the HTML always has a BODY element #410

Merged

oliverklee added a commit that referenced this pull request Oct 13, 2017

[BUGFIX] Make sure the HTML always has a BODY element

94b6cd7

This is a prerequisite to #396.

jjriv pushed a commit that referenced this pull request Oct 13, 2017

[BUGFIX] Make sure the HTML always has a BODY element (#410)

9120505

This is a prerequisite to #396.

oliverklee added 3 commits October 16, 2017 18:58

Merge branch 'master' into pr/1

950dead

Update Emogrifier.php

97d43aa

Update EmogrifierTest.php

a2e55f8

oliverklee approved these changes Oct 16, 2017

View reviewed changes

oliverklee merged commit 351efe2 into MyIntervals:master Oct 16, 2017

oliverklee mentioned this pull request Oct 20, 2017

Move media queries to <body> instead of <head> #358

Closed

TwanVermeulen mentioned this pull request Nov 13, 2018

Stylesheet in body seems to be ignored by gmail #634

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add style blocks to body rather than head v2 #396

Add style blocks to body rather than head v2 #396

Renkas commented Jul 29, 2017 •

edited

Loading

oliverklee left a comment

oliverklee Jul 31, 2017

oliverklee Jul 31, 2017

Renkas Sep 10, 2017

oliverklee Jul 31, 2017

oliverklee Jul 31, 2017

Renkas Sep 10, 2017

oliverklee Sep 10, 2017

oliverklee Jul 31, 2017

oliverklee Jul 31, 2017

oliverklee Jul 31, 2017

oliverklee Jul 31, 2017

oliverklee Jul 31, 2017

oliverklee commented Aug 24, 2017

oliverklee Sep 10, 2017

oliverklee commented Sep 17, 2017

Renkas commented Sep 20, 2017

oliverklee commented Sep 20, 2017

oliverklee commented Oct 13, 2017

oliverklee commented Oct 14, 2017

oliverklee commented Oct 16, 2017

		@@ -1111,7 +1111,7 @@ public function emogrifyKeepExistingHeadElementContent()
		*/
		public function emogrifyKeepExistingHeadElementAddStyleElement()

Add style blocks to body rather than head v2 #396

Add style blocks to body rather than head v2 #396

Conversation

Renkas commented Jul 29, 2017 • edited Loading

oliverklee left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

oliverklee commented Aug 24, 2017

Choose a reason for hiding this comment

oliverklee commented Sep 17, 2017

Renkas commented Sep 20, 2017

oliverklee commented Sep 20, 2017

oliverklee commented Oct 13, 2017

oliverklee commented Oct 14, 2017

oliverklee commented Oct 16, 2017

Renkas commented Jul 29, 2017 •

edited

Loading