Introduce scss linting to codebase

[blyedev]

Description

Disclaimer: this PR is purely the result of the fact i was experimenting with tooling and needed a larger codebase to do so.

In here I add stylelint which allows you to spot issues in your scss such as duplicate selectors, prefixes that shouldn't be there because of tools like autoprefixer and many others. Each issue comes with a rule name which has some solid reasoning behind it on their website.

Judging by the current output it needs fine tuning to adjust to the code-base conventions such as selector-case since there's a lot of classes with camel case. But given scss conventions maintainers might elect to conform to the recommended rules.

Perhaps the coolest part of this PR though: The formatter on the lint:scss:ci script makes GitHub highlight all errors directly in the code so you get something akin to an automatic code review. So if you enable GitHub actions on my PR you will see every error raised without having to run it locally

From the 1800 issues about 800 are auto-fixable, I did not do that in this PR because I do not know this code-base nearly well enough to do so confidently but if I receive approval I will gladly fix all the issues mentioned by the linter

Issues Resolved

List any existing issues this PR resolves

Check List

• New functionality includes testing.
• New functionality has been documented in the README if applicable.

[github-actions]

Hello there blyedev! 👋

Thank you and congrats 🎉 for opening your first PR on this project! ✨ 💖

We will try to review it soon!

[johannesjo]

Hey there! Thank you very much for opening up this PR. While I support adding stylelint I am currently developing a bigger new feature, which I like to merge back to master before doing such a wide ranging change. Would it be alright if I ping you once that is done?

[blyedev]

Sure

[johannesjo]

Hey @blyedev ! The big changes have been merged back to the master branch, so we could start with this, if you're still interested :)

[blyedev]

Hey @blyedev ! The big changes have been merged back to the master branch, so we could start with this, if you're still interested :)

Yes I am. I'll rebase my changes and get on the issues, I might need some time though I'm working on another project concurrently right now.

Ah and regarding the CI I should note that the github formatting only annotates the first 10 errors into files the rest go seemingly unnoticed in PR's but they are still there in the github action, It's your call if you want to keep the feature.

[johannesjo]

Ah and regarding the CI I should note that the github formatting only annotates the first 10 errors into files

I think that is alright. Is the total number of errors shown?

[blyedev]

Ah and regarding the CI I should note that the github formatting only annotates the first 10 errors into files

I think that is alright. Is the total number of errors shown?

Not in the summary of the workflow nor in the PR's files changed. I included this check in this pr so you could check out how it looks like. You can see all the errors in the workflow itself. There will never be a case where there is nothing reported despite actual errors being present but there will not be an indication that theres anything beyond the 10 errors.

EDIT: I managed to find an issue that I originally learned that through: https://github.com/orgs/community/discussions/68471

[blyedev]

I'm also looking over the changes to be made and it seems like a not insignificant portion is a simple whitespace change, if you'd like I could make those fixes a separate commit that we'd put in .git-blame-ignore-revs. That is also a very minor difference that merely prevents me to have seemingly authored half the css in this repo according to git blame

[johannesjo]

if you'd like I could make those fixes a separate commit that we'd put in .git-blame-ignore-revs

That would be great! Which rule is causing this? Maybe there is some way to avoid this? I think in general, we shouldn't be to strict with them (the rules) as most important stuff should be already handled by prettier and things just about formatting stuff should be handled by that mostly.

[blyedev]

if you'd like I could make those fixes a separate commit that we'd put in .git-blame-ignore-revs

That would be great! Which rule is causing this? Maybe there is some way to avoid this? I think in general, we shouldn't be to strict with them (the rules) as most important stuff should be already handled by prettier and things just about formatting stuff should be handled by that mostly.

So far I've seen the comment-whitespace-inside and double-slash-comment-empty-line-before, but I'm sure there's more. I'll do a proper report of potentially troublesome changes once I start working on it. In the meanwhile I'll have to reconstruct the PR because I forgot to work on a feature branch in my fork

[johannesjo]

That sounds good! Generally, I'd say let's start with fewer rules rather than more and only introduce rules that really add value. The project survived without (s)css linting for quite a while now and too many rigid rules might turn off potential contributors to the project.

[blyedev]

You are absolutely right in that regard the balance is crucial, I'll leave deciding that to you.

Right now I'm turning on the rules one by one and creating commits respectively to each rule so after I'm done you can choose which are overly intrusive and I can simply revert a commit. We can even keep the change and just disable this specific rule for future reference for things such as the space with comments.

I have a feeling that after I get 3-4 whitespace rules out of the way the remaining choices will most likely be very sensible. Only time will tell