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

Not properly parsing out comments. #130

Closed
patricknelson opened this issue Mar 30, 2016 · 7 comments
Closed

Not properly parsing out comments. #130

patricknelson opened this issue Mar 30, 2016 · 7 comments

Comments

@patricknelson
Copy link

Hi, I found that this plugin wasn't properly parsing out HTML comments which may exist in some inline <style> blocks, such as this one for example:

<style type="text/css">
<!--
.bwalignc {text-align: center}
// ...more classes...
-->
</style>

As a result, ->processCSS() will end up thinking that the first class above is <!--.bwalignc instead of .bwalignc. I'll submit a quick PR in a moment to patch this up!

@patricknelson
Copy link
Author

Note: It appears this affects v1.5.5 and below. Unfortunately, I'm not seeing a 1.5 branch in your repository that is compatible with this pull request, however due to how this is setup in packagist it's this version that people will be downloading when they user composer to install this (like I did).

I'm setting up a new 1.5 branch where my changes can go. Can you setup a 1.5 branch in your repo as well so this can be merged in somehow?

patricknelson added a commit to patricknelson/CssToInlineStyles that referenced this issue Mar 30, 2016
@patricknelson
Copy link
Author

Here's my change, it's just one line: patricknelson@a4edbab

If that works for you, I can submit the PR after you push an updated branch, and then you can bump the version and slap a tag on it I suppose.

@barryvdh
Copy link
Contributor

Why should you do that anyways? I think that syntax is pretty obsolete, all browsers today fully support stylesheet tags, right?

@patricknelson
Copy link
Author

Yes it is obsolete, but it doesn't prevent large corporate monoliths from distributing code that my clients need to paste into an editor which I must support. I have a specific use case and the workflow needs to be optimized and the time from receipt of HTML to production needs to be reduced and this tool is part of that process :) Plus this opens up the branch for further fixes for anyone else using the older 1.5x API in case there are any other bugs or slight improvements.

@patricknelson
Copy link
Author

And by "time" I'm talking minutes since it is press/news related.

@tijsverkoyen
Copy link
Owner

I won't support 1.5 anymore. I advice you to upgrade to 2.x

@patricknelson
Copy link
Author

Ok, thanks for the update. I know there's some effort involved here on your part (maintaining github) so thanks for your time.

With 1.5 aside: Do you know if v2 properly parses out old-school style HTML comments and, if not, would you be opposed to supporting that if I put the effort into that? Especially if it's not hacky and doesn't have adverse effects anywhere else? 😄

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

No branches or pull requests

3 participants