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

Include SASS-Lint. Improve code standarts, code visibility and fix lint issues for chosen.scss #2489

Closed
wants to merge 2 commits into from

Conversation

taracila
Copy link

I've noticed a lot of issues with code standards in this file.

Initial lint results: https://gist.github.com/taracila/fa241a5e724bf7929bc3
Fixed issues (almost): https://gist.github.com/taracila/60071ce06f490a789d3a

Standarts for SASS code: https://drive.google.com/file/d/0B1AwVhsNLZHYZUo1YVZTby0xUUk/view

Run linter:
run in terminal "scss-lint -c scss-lint.yml"

@taracila taracila changed the title Include SASS-Lint. Improve code standarts, code visibility and fix lint issues. Include SASS-Lint. Improve code standarts, code visibility and fix lint issues for chosen.scss Dec 12, 2015
@tjschuck
Copy link
Member

/cc @mlettini, although I'm going to go ahead and guess this has a pretty low likelihood of getting merged.

See the discussion on #2479 about linters/combers/"standards".

@mlettini
Copy link
Contributor

Thanks for the effort here @taracila.

As @tjschuck mentioned, there's ongoing work and discussion in #2479. That PR should get most of the way there as this one does, by using more variables and alphabetizing property order (I can also add a pass for better comment formatting). But there will be further discussion on property order and linting and the like after that gets merged. It seems it would be beneficial to implement an automatic way of doing that to the repo.

@mlettini mlettini closed this Dec 14, 2015
@taracila
Copy link
Author

Hi @mlettini and @tjschuck

This PR is intended for code formatting (it should not have any regression on any types of selects). I'm using this awesome plugin in each project where I have activated select tag and each time I'm required to reformat the chosen.scss because of background images (replaced by font-icons) and poor formatting code.

BTW, an automatic Continuous Integration service can give a results for all issues/errors with js/sass code pushed by each developer so you can see which PR has issues at the code standards layer.

Thanks for checking that and I'm waiting for a better version of SASS/CSS in chosen plugin in next versions.

@koenpunt
Copy link
Collaborator

Can sass-lint also format the code?
Because if so I guess I prefer using that over CSSComb

@taracila
Copy link
Author

@koenpunt I'm afraid that it's not. I would not agree to have such automatic code formatter let it be php or css. CSSComb has plugins for various IDE and this feature should be managed at local env. Once the code is pushed, the SCSS-lint would check the code and will throw all issues (if there are any). This is the practice that me and another 50~ team members are following.

@koenpunt
Copy link
Collaborator

I'm not really sure why this was closed, because, like @taracila said, it can improve code quality and keeps it consistent when integrated with CI. We could also loosen the rules for linting (I assume that's possible?).
But is this linter also available for node-sass, as we're stripping the ruby dependency altogether in #2483

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.

4 participants