Options for css and js inside html files #1025

Closed
wants to merge 6 commits into
from

Projects

None yet

3 participants

@DaniGuardiola
Contributor

This should be useful for plugins and for developers looking for a more powerful beautifying tool for html

@DaniGuardiola DaniGuardiola Options for css and js inside html files
This should be useful for plugins and for developers looking for a more powerful beautifying tool for html
ecc8244
@DaniGuardiola DaniGuardiola added a commit to DaniGuardiola/Sublime-HTMLPrettify that referenced this pull request Sep 8, 2016
@DaniGuardiola DaniGuardiola Added support for js and css config in html files
This simple change will be very useful if js-beautify maintainers accept my PR to support js and css options inclusion in html files:
beautify-web/js-beautify#1025
ede0fb7
@DaniGuardiola DaniGuardiola referenced this pull request in victorporof/Sublime-HTMLPrettify Sep 8, 2016
Merged

Added support for js and css config in html files #336

DaniGuardiola added some commits Sep 8, 2016
@DaniGuardiola DaniGuardiola This should pass the tests now
39d0030
@DaniGuardiola DaniGuardiola Damn I am sleepy there you go
189c96c
@bitwiseman bitwiseman and 1 other commented on an outdated diff Sep 8, 2016
js/lib/beautify-html.js
@@ -84,7 +84,7 @@
return s.replace(/\s+$/g, '');
}
- function style_html(html_source, options, js_beautify, css_beautify) {
+ function style_html(html_source, options, js_beautify_raw, css_beautify_raw) {
@bitwiseman
bitwiseman Sep 8, 2016 edited Contributor

Instead of changing this, try this below:

var js_beautify_old = js_beautify;
js_beautify = function(js_source_text, options) {
    options = options || options.js;
    js_beautify_old(js_source_text, options);
};
@bitwiseman
bitwiseman Sep 9, 2016 Contributor

But more accurately, I think you'd do better to change each beautifier so each on reads options + options.. Other wise you only solve this for the case where people are running the html beautifier.

@DaniGuardiola
DaniGuardiola Sep 9, 2016 Contributor

Well I don't think anything similar to this will be useful for beautifying js or css as those files will never have blocks with the other two languages as html does

@bitwiseman
bitwiseman Sep 9, 2016 Contributor

But they can all use the same configuration file or options object. So the same schema needs to work for all of them.

@bitwiseman bitwiseman commented on an outdated diff Sep 8, 2016
js/lib/beautify-html.js
+}());
@bitwiseman
bitwiseman Sep 8, 2016 edited Contributor

You must run ./build on you code and commit any changes it makes to ensure your code meets style guidelines for the project.

@bitwiseman
Contributor

@DaniGuardiola - I'm glad to see someone working on this. Thanks!

@bitwiseman bitwiseman commented on an outdated diff Sep 9, 2016
js/lib/beautify-html.js
@@ -108,6 +108,24 @@
options = options || {};
+ // Allow the inclusion of css and js options for <style> and <script> blocks
+ // inside html files (this is useful for plugins like Sublime's HTML prettify)
+ if (!options.js) {
+ options.js = {};
+ }
+ var js_beautify = function(js_source_text, options) {
+ options = options || options.js;
@bitwiseman
bitwiseman Sep 9, 2016 edited Contributor

This is an interesting start, but what you really want is something that overlays any values in options.js onto options object.

And don't forget to do the same with options.html.

So, for instance your the following settings should work and result in html files with 8 space indents, <style> blocks wth 4 space indent, and <script> blocks with 2-space indents.

{
    "indent_size": 5,
    "js": {
        "indent_size": 2
    },
    "html": {
        "indent_size": 8
    }
}
DaniGuardiola added some commits Sep 9, 2016
@DaniGuardiola DaniGuardiola Fixed styles and errors, stilized code
d3acf16
@DaniGuardiola DaniGuardiola Passing the tests
74e7602
@DaniGuardiola DaniGuardiola After build
eb6ab3c
@victorporof victorporof added a commit to victorporof/Sublime-HTMLPrettify that referenced this pull request Sep 9, 2016
@DaniGuardiola @victorporof DaniGuardiola + victorporof Added support for js and css config in html files (#336)
This simple change will be very useful if js-beautify maintainers accept my PR to support js and css options inclusion in html files:
beautify-web/js-beautify#1025
96af936
@bitwiseman
Contributor

@DaniGuardiola - Great! What you have doesn't break the existing tests, now we need tests showing the new feature working. Thanks!

@HookyQR
Contributor
HookyQR commented Nov 4, 2016 edited

I would prefer to see a merge of the options at the embedded level, so things don't have to be defined twice. So that only specifically different settings need to be defined at the embedded level(s).

For instance:

{
    "indent_with_tabs": false,
    "brace_style": "collapse",
    "max_preserve_newlines": 2,
    "js":{
        "indent_with_tabs": true
    }
}

The javascript beautify would only get the indent_with_tabs setting with the current code, and drop the brace_style and max_preserve_newlines.

So instead of calling opt = options.js || opt call opt = mergeOpts(options, "js")

function mergeOpts(allOptions, targetType) {
    var finalOpts = {}, name;
    for (name in allOptions) {
        if (name !== 'js' && name !== 'html' && name !== 'css') {
            finalOpts[name] = allOptions[name];
        }
    }
    //merge in the per type settings for the targetType
    if (targetType in allOptions) {
        for (name in allOptions[targetType]) {
            finalOpts[name] = allOptions[targetType][name];
        }
    }
    return finalOpts;
}

This is the merge I'm using in VSCode Beautify.

@HookyQR
Contributor
HookyQR commented Nov 4, 2016

The function overwrite also circumvents these tests, and will break execution if, for some reason, the import(s) failed.

if (multi_parser.token_type === 'TK_SCRIPT') {
    _beautifier = typeof js_beautify === 'function' && js_beautify;
} else if (multi_parser.token_type === 'TK_STYLE') {
    _beautifier = typeof css_beautify === 'function' && css_beautify;
}
@bitwiseman bitwiseman added this to the v1.6.5 milestone Dec 9, 2016
@bitwiseman
Contributor

Taking the code from this and integrating elsewhere.

@bitwiseman bitwiseman closed this Dec 18, 2016
@DaniGuardiola
Contributor

@bitwiseman hey sorry, this was on my to-do list for a long time, can I help?

@bitwiseman bitwiseman removed this from the v1.6.5 milestone Dec 18, 2016
@bitwiseman
Contributor

@DaniGuardiola @HookyQR - You are welcome to look at the recently merged commits, sync master, and see if the behavior suits your needs.

1c21d5b

It was a bit more involved to get it wired through and tested throughout.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment