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

newline_between_rules support for Sass (enhancement) #657

Closed
joseluisq opened this issue Mar 19, 2015 · 49 comments
Closed

newline_between_rules support for Sass (enhancement) #657

joseluisq opened this issue Mar 19, 2015 · 49 comments

Comments

@joseluisq
Copy link

Now newline_between_rules option in v1.5.5 is supported for CSS only #574
So, in Sass for nesting doesn't work.

Here my test.js file:

var fs = require('fs');
var beautify_css = require('js-beautify').css;

fs.readFile('test.scss', 'utf8', function(err, data) {
  if (err) {
    throw err;
  }

  console.log(beautify_css(data, {
    indent_size: 2,
    newline_between_rules: true
  }));
});

Output:

$ node test.js

.icons {
  padding: 0;
  li {
    display: inline-block;
  }
  a {
    display: block;
    color: #000;
  }
  a:hover {
    color: #ccc;
  }
}

I hope to add newline_between_rules support for Sass.
Regards.

@bitwiseman
Copy link
Member

And what do you want it to look like?

@joseluisq
Copy link
Author

Similar to CSS version #574
In general, one nesting level is enough.
Here an example:

// Icons
.icons {
  padding: 0;

  li {
    display: inline-block;
  }

  a {
    display: block;
    color: #000;
  }

  a:hover {
    color: #ccc;
  }
}

// Button
.button {
  &.primary {
    color: #4183c4;
  }

  &.primary:hover {
    color: lighten(#4183c4, 15%);
  }
}

@deepak8167
Copy link

Hi,
Is this work for scss rules.

.old_price {
@include GibsonRegular();
@include font-size(14);
color: #646464;
margin-left: 10px;
text-decoration: line-through;

.promovalue {
  @include GibsonRegular();
}

}

@echenley
Copy link

echenley commented Oct 9, 2015

+1

3 similar comments
@explorador
Copy link

+1

@chipcullen
Copy link

+1

@spacedawwwg
Copy link

+1

@spacedawwwg
Copy link

maybe an option like newline_between_nested_rules: true as it seems to be specifically an issue with nested?

} and ; would need to consider the rule for nested new lines

@Billskate
Copy link

+1, also for less

@ms-carterk
Copy link

+1

1 similar comment
@nevace
Copy link

nevace commented Aug 11, 2016

+1

@Jakobud
Copy link

Jakobud commented Sep 12, 2016

I can confirm that this is indeed a problem only with the nested rules. Current with newline_between_rules: true this:

body {
  background-color: #FFF;

  > div {
    background-color: #AAA;
  }
}

.container {
  color: blue;
}

Becomes this:

body {
  background-color: #FFF;
  > div {
    background-color: #AAA;
  }
}

.container {
  color: blue;
}

And newline_between_rules: false this is the result:

body {
  background-color: #FFF;
  > div {
    background-color: #AAA;
  }
}
.container {
  color: blue;
}

So support for newlines between nested rules should be considered.

@danieloprado
Copy link

+1

@gligoran
Copy link

Any ETA on this thing?

@francislanglois
Copy link

+1

1 similar comment
@chenxingcai07
Copy link

+1

@futurliberta
Copy link

+1 :(

@kris-owens
Copy link

+1

@zimmerboy
Copy link

Another +1

Sorry for the noise. I didn't know if another way to upvote this.

@gligoran
Copy link

@zimmerboy there's an "Add your reaction" button at the top right of every comment, which contains the +1 reaction (:+1:).

@patroza
Copy link

patroza commented Jan 13, 2017

My workaround in beautify-css.js:

  1. Replace newLine function:
        print.newLine = function(keepWhitespace, keepNewLine) {
            if (output.length) {
                if (!keepWhitespace && output[output.length - 1] !== '\n') {
                    print.trim();
                    if (keepNewLine) { output.push('\n'); }
                }

                output.push('\n');

                if (basebaseIndentString) {
                    output.push(basebaseIndentString);
                }
            }
        };
  1. Change both newline_between_rules conditions and statements to:
                    if (newline_between_rules) {
                        print.newLine(false, true);
                    }

Note: This only takes care of newlines between rules, not rules after properties. Anyone some ideas on that?

@bitwiseman
Copy link
Member

@Sickboy - Cool, start a pull request, add some tests. Please feel free to move this forward.

@mrahhal
Copy link

mrahhal commented Jan 21, 2017

Is this coming anytime soon? It's my only blocker for using beautify for sass.

@bitwiseman
Copy link
Member

@mrahhal - The issue is nearly two years old. It seams @Sickboy has done some work on this. Someone just needs to make a pull request with tests.

Maybe you'd like to do this? See CONTRIBUTING.md.

@mrahhal
Copy link

mrahhal commented Jan 21, 2017

@bitwiseman unfortunately I don't have the experience with this kind of projects nor the time. But my question is, so is everyone either using plain css or just sticking with how the current formatting works? That's weird for a very old but easy to solve issue .

@Sickboy are you on this? Maybe I'll tackle it after all if nobody's active on this. Apart from some weird behavior with "max_preserve_newlines", this is really the only thing missing that's completely preventing me from using beautify.

@royduin
Copy link

royduin commented Apr 7, 2017

Any updates?

@bitwiseman
Copy link
Member

@royduin - please test with the latest release.

@stgogm
Copy link

stgogm commented Apr 17, 2017

It seems to work fine but there's a little issue with comments. I know Bootstrap SCSS is not a great example, but still.

This is how the code is formatted on the repo:
image

And this is after beautifying:
image

It works as expected but comments overlap with previous declaration, but if comments are formatted correctly, it works fine:
image

Again, it's nothing really important, but Bootstrap is widely used 😕

@bitwiseman
Copy link
Member

@stgogm
Please file a new issue.
Also, please include text, not images.

@stgogm
Copy link

stgogm commented Apr 18, 2017

@bitwiseman Okay, sorry. Just wanted to note that as it's not really an issue if you respect the SCSS linter rules.

@bitwiseman
Copy link
Member

@stgogm - Cool, thanks for the info. 😄

@olets
Copy link

olets commented Apr 19, 2017

@stgogm while you have it set up, could you test the nested newline_between_rules issue? With

newline_between_rules: true

does your

fieldset {
    border: 0;
    margin: 0;
    min-width: 0;
    padding: 0;
    &+fieldset {
        margin-top: $padding-large;
    }
}

become

fieldset {
    border: 0;
    margin: 0;
    min-width: 0;
    padding: 0;

    &+fieldset {
        margin-top: $padding-large;
    }
}

?

@stgogm
Copy link

stgogm commented Apr 20, 2017

Actually, it makes no difference:

Code as authored (without new line in between):

form {
  display: block;
}

fieldset {
  border: 0;
  margin: 0;
  min-width: 0;
  padding: 0;
  & + fieldset {
    margin-top: $padding-large;
  }
}

After beautify with "newline_between_rules": false:

form {
  display: block;
}

fieldset {
  border: 0;
  margin: 0;
  min-width: 0;
  padding: 0;
  &+fieldset {
    margin-top: $padding-large;
  }
}

After beautify with "newline_between_rules": true:

form {
  display: block;
}

fieldset {
  border: 0;
  margin: 0;
  min-width: 0;
  padding: 0;
  &+fieldset {
    margin-top: $padding-large;
  }
}

It just removed the space between the +.

js-beautify --version
1.6.12

@olets
Copy link

olets commented Apr 21, 2017

Thanks @stgogm! So nope @bitwiseman #1146 didn't fix this

@vvs
Copy link

vvs commented Jun 18, 2017

Another example where "newline_between_rules" is not enough for SCSS:

$variable: #333;
div {
  display: none;
}

Formatting this SCSS doesn't add a new line between the variable and the div selector.

@dehghani-mehdi
Copy link

Is clear when this feature will be available?

@whxaxes
Copy link

whxaxes commented Jul 15, 2017

Does it have any progress?

@bitwiseman
Copy link
Member

@dehghani-mehdi @whxaxes
This feature is unassigned. It needs someone to implement it and add tests.

@giubatt
Copy link

giubatt commented Aug 22, 2017

+1

4 similar comments
@RenaldasK
Copy link

+1

@martinsimonovski
Copy link

+1

@nerdmax
Copy link

nerdmax commented Sep 3, 2017

+1

@ciel
Copy link

ciel commented Sep 22, 2017

+1

@olets
Copy link

olets commented Sep 22, 2017

Please vote with a 👍 reaction to the initial post rather than +1 messages - that won't spam subscribers. Thanks!

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

No branches or pull requests