Skip to content

Conversation

@tijsverkoyen
Copy link
Owner

The 2.0 version is a major overhaul, which is not backwards compatible.

  • From now on you can re-use the class for multiple mails.
  • A lot less complicated options, as in: no more options at all.
  • More separate classes which handle their own (tested) methods.
  • A lot more tests

The reason why I did this was to make the class more usable.

…ble.

* From now on you can re-use the class for multiple mails.
* A lot less complicated options, as in: no more options at all.
* More separate classes which handle their own (tested) methods.
* A lot more tests

The reason why I did this was to made the class more usable.
@tijsverkoyen
Copy link
Owner Author

@barryvdh , @stof , @GrahamCampbell would you guys be so kind to have a look at the v2.0.0-PR. It is a major change, but I think it is a nice improvement.

Some stuff has been removed, feel free to tell me what is missing, and I will implement it again.

Thanks in advance.

@voku
Copy link

voku commented Nov 22, 2015

The code for the clean-up (cleanup() | doCleanup()) is copy&pasted three times in the project, maybe we can create one method for this task ... maybe it's also a good idea to move the reg-ex into a static variable, so it has a name and we can read it and we can quickly edit/fix it. E.g. the reg-ex for code-comments in the next "globalCleanup()" method covered also comments (/** foobar */) in CSS files. (here is the test: https://github.com/voku/CssToInlineStyles/blob/master/tests/CssToInlineStylesTest.php#L187)

/**
 * @param $string
 * @return mixed|string
 */
protected function cleanup($string)
{
    $string = str_replace(array("\r", "\n"), '', $string);
    $string = str_replace(array("\t"), ' ', $string);
    $string = str_replace('"', '\'', $string);
    $string = preg_replace('|/\*.*?\*/|', '', $string);
    $string = preg_replace('/\s\s+/', ' ', $string);

    $string = trim($string);
    $string = rtrim($string, ';');

    return $string;
}

protected function cleanup($string)
{
    $string = str_replace(array("\r", "\n"), '', $string);
    $string = str_replace(array("\t"), ' ', $string);
    $string = str_replace('"', '\'', $string);
    $string = preg_replace('|/\*.*?\*/|', '', $string);
    $string = preg_replace('/\s\s+/', ' ', $string);

    $string = trim($string);
    $string = rtrim($string, '}');

    return $string;
}

/**
 * @param $css
 * @return mixed|string
 */
protected function doCleanup($css)
{
    // remove media queries
    $css = preg_replace('/@media [^{]*{([^{}]|{[^{}]*})*}/', '', $css);

    $css = str_replace(array("\r", "\n"), '', $css);
    $css = str_replace(array("\t"), ' ', $css);
    $css = str_replace('"', '\'', $css);
    $css = preg_replace('|/\*.*?\*/|', '', $css);
    $css = preg_replace('/\s\s+/', ' ', $css);
    $css = trim($css);

    return $css;
}

into -->

/**
 * @param string $string
 * @param string $rtrim
 * @param bool   $removeMediaQueries
 * @return string
 */
protected function globalCleanup($string, $rtrim = '', $removeMediaQueries = false)
{
    // remove media queries
    if ($removeMediaQueries === true) {
      $string = preg_replace('/@media [^{]*{([^{}]|{[^{}]*})*}/', '', $string);
    }

    // remove newlines & tab to space & replace double quotes by single quotes
    $string = str_replace(array("\r", "\n", "\t", '"'),array('', '', ' ', '\''),$string);

    // remove comments
    $string = preg_replace('/\\/\\*.*\\*\\//sU', '', $string);

    // remove spaces
    $string = preg_replace('/\s\s+/', ' ', $string);

    $string = trim($string);

    if ($rtrim) {
      $string = rtrim($string, $rtrim);
    }

    return $string;
}

@barryvdh
Copy link
Contributor

So what options are now used by default? I usually need the following:

  • Convert HTML with CSS in the head (no separate css file)
  • Strip the original css from the head, after inlining.
    • Keep the media queries, because they cannot be inlined.

composer.json Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

You should keep the branch alias IMO (updating it to 2.0)

Copy link
Collaborator

Choose a reason for hiding this comment

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

this should be kept (with the update), so that the master branch has the right alias after merging this to master

Copy link
Owner Author

Choose a reason for hiding this comment

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

fixed in de745ff

@stof
Copy link
Collaborator

stof commented Nov 23, 2015

@tijsverkoyen can you describe what is the 1.x equivalent configuration of the new library ? This would help knowing what feature is kept and what is removed

composer.json Outdated
Copy link
Collaborator

Choose a reason for hiding this comment

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

you should also allow phpunit 5, not only 4.8 (4.8 does not officially support PHP 7, even if it mostly works)

Copy link
Owner Author

Choose a reason for hiding this comment

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

fixed in 2851b4a

@tijsverkoyen
Copy link
Owner Author

I think most of the feedback is processed. Could you all test with real examples to see if everything is working as expected?

@barryvdh
Copy link
Contributor

barryvdh commented Dec 8, 2015

Tried rendering all the Zurb examples from here: http://foundation.zurb.com/emails/email-templates.html
Apart from #119 it seems to be working.

barryvdh and others added 2 commits December 8, 2015 16:58
@tijsverkoyen
Copy link
Owner Author

@barryvdh thx!

@barryvdh
Copy link
Contributor

So, this good to go?

@tijsverkoyen
Copy link
Owner Author

@barryvdh if nobody has any remarks anymore ;-)
I will try to release a new version this week

@barryvdh
Copy link
Contributor

Perhaps tag it as an alpha release first, so we can spot some issues with the public API, in case anyone missed something :)

@tijsverkoyen tijsverkoyen merged commit 4b9c31b into master Feb 23, 2016
@tijsverkoyen tijsverkoyen deleted the 2.0 branch September 20, 2016 12:25
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.

9 participants