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

Add content_unformatted config to avoid formatting content of an element (fix #906) #1080

Merged
merged 1 commit into from
Dec 28, 2016

Conversation

arai-a
Copy link
Contributor

@arai-a arai-a commented Dec 25, 2016

This fixes #906

The issue is that unformatted config is applied to following 3 places:

  • newline before the element
  • content of the element
  • newline after the element

while it's useful for inline elements, when one want to disable formatting only the content of the content, keeping newlines before/after it, the config cannot be used.

I added new content_unformatted config, that does only "content of the element" part.
if content_unformatted one add "style" and "script" to this, their content is not formatted, while they still have newlines before/after them.

config file:

{
    "content_unformatted": [ "script", "style" ]
}

output

$ node ./js/bin/html-beautify.js   --config ~/Desktop/conf ~/Desktop/b.html
<body>
    <h1>Heading</h1>
    <script>var formatMe = function() { return false; };</script>
    <style>.format-disabled { display: none; } </style>
</body>

I also added commandline option --content_unformatted, or -T (the first character in 'content_unformatted' that wasn't used...), that can be used like --unformatted.

$ node ./js/bin/html-beautify.js -T script ~/Desktop/b.html
<body>
    <h1>Heading</h1>
    <script>var formatMe = function() { return false; };</script>
    <style>
        .format-disabled {
            display: none;
        }
    </style>
</body>

$ node ./js/bin/html-beautify.js -T style ~/Desktop/b.html
<body>
    <h1>Heading</h1>
    <script>
        var formatMe = function() {
            return false;
        };
    </script>
    <style>.format-disabled { display: none; } </style>
</body>

$ node ./js/bin/html-beautify.js -T script -T style ~/Desktop/b.html
<body>
    <h1>Heading</h1>
    <script>var formatMe = function() { return false; };</script>
    <style>.format-disabled { display: none; } </style>
</body>

@arai-a arai-a changed the title Add content_unformatted config to avoid formatting content of an elem… Add content_unformatted config to avoid formatting content of an element (fix #906) Dec 25, 2016
@bitwiseman
Copy link
Member

@arai-a - Thanks for submitting.
Please rebase the latest changes.
Also, look at the tests for #1081 and make sure you handle those inputs, as well.
Your change looks simpler than that one as well.

Finally, please format your multi-line test inputs or outputs like this to make them easier to understand:

        tests: [{
            fragment: true,
            input: '<html><body><h1>A</h1><script>if(1){f();}</script><style>.a{display:none;}</style></body></html>',
            output: [
                '<html>',
                '<body>',
                '    <h1>A</h1>',
                '    <script>if(1){f();}</script>',
                '    <style>.a{display:none;}</style>',
                '</body>',
                '',
                '</html>'
            ]
        }]

@arai-a
Copy link
Contributor Author

arai-a commented Dec 26, 2016

rebased onto latest master and added tests from #1081 and it passed without any changes.
thank you!

Copy link
Member

@bitwiseman bitwiseman left a comment

Choose a reason for hiding this comment

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

A few more tests needed and need to have pre as part of the default. Once that is done, this looks good.

@@ -161,6 +163,7 @@
'acronym', 'address', 'big', 'dt', 'ins', 'strike', 'tt',
'pre',
];
content_unformatted = options.content_unformatted || [];
Copy link
Member

Choose a reason for hiding this comment

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

Please move 'pre' from unformatted, into the default for content_unformatted and add some tests for it based on #928.

name: "content_unformatted to prevent formatting content",
description: "",
options: [
{ name: 'content_unformatted', value: "['script', 'style', 'p', 'span', 'br']" }
Copy link
Member

Choose a reason for hiding this comment

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

Add some examples with <pre> tag (as mentioned above).
Then please do the same inputs with the default content_unformatted. It is important to understand how this changes the outputs.

@arai-a arai-a force-pushed the content_unformatted branch from b3798f1 to 32c8e6e Compare December 27, 2016 00:51
@arai-a
Copy link
Contributor Author

arai-a commented Dec 27, 2016

okay, moved pre to content_unformatted and added tests for pre and default options,
and also updated one pre-existing test to follow the change to pre

@bitwiseman
Copy link
Member

Great, thanks.
There's been a question from #1081. I need to establish whether this issue raised there is valid or not before we merge. Other than that this looks good.

@arai-a
Copy link
Contributor Author

arai-a commented Dec 27, 2016

I don't see the missing newline issue, on osx, node v6.9.1.
I get expected result with cli.

@arai-a
Copy link
Contributor Author

arai-a commented Dec 27, 2016

tested also on debian, nodejs v4.6.1,
I get expected result.

@bitwiseman bitwiseman merged commit fb13bcb into beautifier:master Dec 28, 2016
@bitwiseman
Copy link
Member

Thank you for your contributio. I hope you will continue to contribute beyond this one PR.

@arai-a
Copy link
Contributor Author

arai-a commented Dec 29, 2016

thank you too for reviewing :)
It's my pleasure to be able to contribute to software I use daily.

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.

Beautify script/style tags but ignore their inner JS/CSS content
2 participants