Skip to:
Content

BuddyPress.org

Opened 5 years ago

Closed 5 years ago

Last modified 4 years ago

#6863 closed defect (bug) (fixed)

Indentation fixes for `.scss` files

Reported by: netweb Owned by: hnla
Milestone: 2.5 Priority: normal
Severity: normal Version:
Component: Core Keywords: has-patch
Cc:

Description

As noted in Slack there are a handful of indentation fixes needed for the Twenty* .scss stylesheets.

The SASS Ruby gem doesn't detect indentation as well as it should sadly, attached patch fixes

  • Mixed indentation of tabs and spaces is not correctly detected
  • Indentation level not detected correctly
  • Indentation level of comments not detected correctly

Attachments (1)

6863.diff (13.1 KB) - added by netweb 5 years ago.

Download all attachments as: .zip

Change History (10)

@netweb
5 years ago

#1 @DJPaul
5 years ago

Thanks for patching these!

#2 follow-up: @hnla
5 years ago

@netweb

On the recent twentyfifteen.scss update I see indentation errors with some rulesets introduced but the assertion: Indentation level not detected correctly puzzles me as when writing and running lint on files it will absolutely pick up on any incorrect indentation level for nested rulesets, yet when I run scsslint on the file where I can see obvious indentation errors the lint task passes without picking up those errors.

Have we any idea why scsslint is behaving this way? It feels as though it only checks a file accurately if it has actual changes.

I think this precipitates us getting shot of the ruby gem approach as soon as possible.

#3 in reply to: ↑ 2 @netweb
5 years ago

Replying to hnla:

@netweb

On the recent twentyfifteen.scss update I see indentation errors with some rulesets introduced but the assertion: Indentation level not detected correctly puzzles me as when writing and running lint on files it will absolutely pick up on any incorrect indentation level for nested rulesets, yet when I run scsslint on the file where I can see obvious indentation errors the lint task passes without picking up those errors.

I just don't think it is "smart" enough to catch everything it is asked to

Have we any idea why scsslint is behaving this way?

Mixed indentation of tabs and spaces is not correctly detected is the primary reason from what I can see, it expects 2 indents, and if 1 is a tab and 1 is a space then it counts that as 2 and marks it correct when in fact it is not correct.

It feels as though it only checks a file accurately if it has actual changes.

Its a time thing, as the scss files have been being linted for a while now the ruleset that they are checked upon are now becoming quite finely honed, and as such its now when an error is noticed its a matter of taking a fine tooth comb to both the configuration and the code to see what the issue is.

I get quite a bit of enjoyment and have a boat load of fun digging around these sorts of things ;)

#4 follow-up: @hnla
5 years ago

  • Owner set to hnla
  • Status changed from new to accepted

:)

I get quite a bit of enjoyment and have a boat load of fun digging around these sorts of things

Wish I could say the same, or would do if I had that time. Sorting out a better linter though now a priority, when I have a minute I'll look at the suggestion you made on that other ticket where scsslint was an issue.

Thanks for the patch, I've actually had to go through the files as looking at these issues, suggested it was better to delete the commented out rulesets/properties for neatness and sadly and although I had accounted for the majority of css comments a few still existed where scss fails to be able to handle them as comments in nested selectors.

I'll commit changes in due course.

#5 in reply to: ↑ 4 @netweb
5 years ago

Replying to hnla:

:)

I get quite a bit of enjoyment and have a boat load of fun digging around these sorts of things

Wish I could say the same, or would do if I had that time. Sorting out a better linter though now a priority, when I have a minute I'll look at the suggestion you made on that other ticket where scsslint was an issue.

The patch 6863.diff was created using Stylelint, what I'm proposing to use as the replacement to the Ruby gem.

I'll create a ticket and patch tomorrow for Stylelint and various other PostCSS plugins to enhance BP's CSS.

Thanks for the patch, I've actually had to go through the files as looking at these issues, suggested it was better to delete the commented out rulesets/properties for neatness and sadly and although I had accounted for the majority of css comments a few still existed where scss fails to be able to handle them as comments in nested selectors.

I'll commit changes in due course.

What exactly is wrong with the comments per the attached patch? They are now nested per WordPress CSS coding standards.

In my opinion the patch is fine as is, it is based on the WordPress CSS coding standards and the configuration I have created for Stylelint. It is quite concise and I've been constantly iterating on for ~6 months now. If something doesn't add up please let me know so any changes you think are needed can be added upstream as needed :)

#6 @hnla
5 years ago

  • Resolution set to fixed
  • Status changed from accepted to closed

In 10493:

Companion styles scss tidy ups

  • Remove commented out properties/rulesets.
  • Correct indentations.
  • Remove stray spaces on indents.
  • Remove CSS style comments from nested selectors.

Fixes #6863 Props netweb

#7 follow-up: @hnla
5 years ago

I'll create a ticket and patch tomorrow for Stylelint and various other PostCSS plugins to enhance BP's CSS.

Great stuff!

What exactly is wrong with the comments per the attached patch? They are now nested per WordPress CSS coding standards.

:) No I think we're at cross purposes here, I'm referring to css comments if you add css comments within sass nested rulesets the compiler sees the comment as a property and expands it, you end up with:

selector {
 /* the comment */
}

Also I decided that it was better to remove commented out (not compiled) scss rulesets.

This wasn't about coding standards per se, however please do base your scss stylelint rules on the .scs-lint.yml we have as we had a few changes in that.

#8 in reply to: ↑ 7 @netweb
5 years ago

Replying to hnla:

:) No I think we're at cross purposes here, I'm referring to css comments if you add css comments within sass nested rulesets the compiler sees the comment as a property and expands it, you end up with:

selector {
 /* the comment */
}

Ahhh... Thanks for clarifying that.

Also I decided that it was better to remove commented out (not compiled) scss rulesets.

Cool

This wasn't about coding standards per se, however please do base your scss stylelint rules on the .scs-lint.yml we have as we had a few changes in that.

It is, the .scss-lint.yml and the Stylelint rules are all based WordPress' CSS coding standards

#9 @DJPaul
4 years ago

  • Component changed from Tools - Code Improvement to Core
Note: See TracTickets for help on using tickets.