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

Code Style #266

Closed
Garbee opened this issue Apr 30, 2015 · 7 comments
Closed

Code Style #266

Garbee opened this issue Apr 30, 2015 · 7 comments

Comments

@Garbee
Copy link
Collaborator

Garbee commented Apr 30, 2015

In some places of the Scss we have no alignment for colons, while in other places there is alignment.

Is there any set style for the projects code?

Further, we should setup tooling to check the code style through a gulp task. Maybe even using something like gulp-csslint to verify the output CSS adhears to some set rules.

@addyosmani
Copy link
Contributor

I agree. Code style checking is probably something we should be doing as part of the build for both JS and Sass/CSS. My personal preference for Sass is that we do use alignment. It's clean and relatively easy to read. @surma @sgomes @brianhall any opinions?

I haven't used gulp-csslint heavily before, but if someone has experience with it and would be interested in playing around a PR, I certainly wouldn't turn that down.

@sgomes
Copy link
Contributor

sgomes commented May 1, 2015

I'm usually against aligning all values to a given column, for a few reasons:

  • It's heavily dependent on which properties you're using (indent = largest property name), which means:
    • Indenting is usually different between different selectors
    • Copy and paste within a file is tedious, due to reindenting at different levels
    • You often have to do meaningless changes to indenting when adding a new property with a longer name
  • It makes it easier to reach line length limits, forcing us to either break/increase them or add newlines
  • It doesn't improve readability that much, IMO

That said, I don't feel so strongly about this that I'd go against the majority.

@Garbee
Copy link
Collaborator Author

Garbee commented May 1, 2015

I don't care either way. But I'd rather not align just to save us all the time of having to align if we don't care to find tooling that will do it.

csslint is also more for the technological output over the style. i.e. Making sure you have vendor prefix and in the right order. Something else would be needed for exact style of the scss.

@addyosmani
Copy link
Contributor

I started exploring https://github.com/brigade/scss-lint this evening and so far really like it. It has task runner and text editor plugins for most setups readily available and as one might expect, a config file. As we haven't been running our styles against any linters, we would probably want to start off with most options switched off and gradually turn them on as we improve. One possible config to start us off (if we like the tool):

scss_files: 'src/**/*.scss'

linters:
  BorderZero:
    enabled: false
  ColorKeyword:
    enabled: false
  ColorVariable:
    enabled: false
  Comment:
    enabled: false
  DeclarationOrder:
    enabled: false
  DuplicateProperty:
    enabled: false
  EmptyLineBetweenBlocks:
    enabled: false
  HexLength:
    enabled: false
  HexNotation:
    enabled: false
  IdSelector:
    enabled: false
  ImportPath:
    enabled: false
  ImportantRule:
    enabled: false
  Indentation:
    enabled: true
    width: 2
    character: space
  LeadingZero:
    enabled: false
  MergeableSelector:
    enabled: false
  NameFormat:
    enabled: false
  NestingDepth:
    enabled: false
  PropertySortOrder:
    enabled: false
  PropertySpelling:
    enabled: false
  QualifyingElement:
    enabled: false
  SelectorDepth:
    enabled: false
  SelectorFormat:
    enabled: false
  Shorthand:
    enabled: false
  SingleLinePerSelector:
    enabled: false
  SpaceAfterComma:
    enabled: false
  SpaceAfterPropertyColon:
    enabled: false
  SpaceAfterPropertyName:
    enabled: false
  SpaceBeforeBrace:
    enabled: false
  SpaceBetweenParens:
    enabled: false
  StringQuotes:
    enabled: true
  TrailingSemicolon:
    enabled: false
  UnnecessaryMantissa:
    enabled: false
  UnnecessaryParentReference:
    enabled: false
  UrlQuotes:
    enabled: false
  VendorPrefix:
    enabled: true
  ZeroUnit:
    enabled: false

@yuyokk
Copy link
Contributor

yuyokk commented Jul 8, 2015

As a suggestion for CSS code style to follow some css guides especially 'Declaration order' by Mark Otto. The CSS becomes more readable :)

@innerpeace0
Copy link

Great reference @yuyokk. Thanks.

@sgomes
Copy link
Contributor

sgomes commented Jul 1, 2016

Superseded by the linting setup in #4464.

@sgomes sgomes closed this as completed Jul 1, 2016
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

5 participants