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

CssInliner in release 3.0.0 #680

Closed
oliverklee opened this issue Aug 29, 2019 · 2 comments · Fixed by #701
Closed

CssInliner in release 3.0.0 #680

oliverklee opened this issue Aug 29, 2019 · 2 comments · Fixed by #701
Assignees
Milestone

Comments

@oliverklee
Copy link
Contributor

  1. Should we mark CssInliner as the new default (instead of as as tech preview) and mark the Emogrifier class as deprecated in 3.0?
  2. As 3.0 is per semantic-versioning definition a breaking release (and CssInliner a tech preview before 3.0), do you think ti would be okay if we removed the automatic removal of invisible nodes from CssInliner for 3.0?
@oliverklee oliverklee added this to the 3.0.0 milestone Aug 29, 2019
@oliverklee oliverklee self-assigned this Aug 29, 2019
@oliverklee
Copy link
Contributor Author

And we have this remark in the README anyway:

$emogrifier->disableInvisibleNodeRemoval() - By default, Emogrifier removes
elements from the DOM that have the style attribute display: none;. If
you would like to keep invisible elements in the DOM, use this option.
Note: This option will be removed in Emogrifier 3.0. HTML tags with
display: none; then will always be retained.

@JakeQZ
Copy link
Contributor

JakeQZ commented Aug 30, 2019

  1. Should we mark CssInliner as the new default (instead of as as tech preview) and mark the Emogrifier class as deprecated in 3.0?

I think so.

  1. As 3.0 is per semantic-versioning definition a breaking release (and CssInliner a tech preview before 3.0), do you think ti would be okay if we removed the automatic removal of invisible nodes from CssInliner for 3.0?

As it's a tech preview before 3.0, we could even remove it for 2.2 (if there is a 2.2) - along the lines of the semantics for pre-1.0 releases (i.e. version 0.2 may include breaking changes from 0.1 though 0.1.1 may not). And anyway, we have already changed the way CssInliner instances are created since 2.1.x, which is a breaking change (new CssInliner($html) -> CssInliner::fromHtml($html)).

Note: This option will be removed in Emogrifier 3.0. HTML tags with
display: none; then will always be retained.

Though as the Emogrifier class will be marked deprecated, we can probably leave it in that class, just remove it from CssInliner.

oliverklee added a commit that referenced this issue Sep 11, 2019
The `CssInliner` class now is not a tech preview anymore, and it should
be used instead.

The documentation in the README will be enhanced and polished even more
in a separate change.

Closes #680
Part of #695
oliverklee added a commit that referenced this issue Sep 11, 2019
The `CssInliner` class now is not a tech preview anymore, and it should
be used instead.

The documentation in the README will be enhanced and polished even more
in a separate change.

Closes #680
Part of #695
JakeQZ pushed a commit that referenced this issue Sep 11, 2019
[TASK] Deprecate the Emogrifier class

The `CssInliner` class now is not a tech preview anymore, and it should
be used instead.

The documentation in the README will be enhanced and polished even more
in a separate change.

Closes #680
Part of #695
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants