build(stylelint): add linting to scss via stylelint, and fix all current fails

[blyedev]

Supersedes #3703

Previously this PR used my forks master branch which was troublesome for rebasing to keep up with fresh features. Discussions are reiterated here for clarity.

Description

This PR adds the stylelint linter ensuring no duplicate selectors, obsolete prefixes, and other easy to miss scss mistakes. It also will fix existing issues to ensure master remains evergreen.

The lint and test workflow step introduced here uses the github formatter which creates annotations on PR's. The amount of annotations are silently limited to 10 pro step in a workflow so if the count is 1900 which is the case at the time of writing only 10 will be displayed in the files changed tab and in the workflow summary. All of them are still displayed in the details of the workflow run. The workflow will never pass despite linting fails existing so only the question if this behavior is misleading remains.

White-space only changes will be put in .git-blame-ignore-revs to ensure only meaningful changes are displayed in git blame with my username.

I will go through all rules sequentially to ensure significant changes are discussed and the stylelint config potentially adjusted.

[blyedev]

Adding to my previous reponse from the previous pr on this:

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.

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

A lot of the rules on there like color-function-notation have autofixes. I'd argue that if we put them into precommit hooks, they don't introduce any mental overhead for new contributors while still keeping in line with best practices. Simmilairly to how formatting is set up.

[johannesjo]

Thank you very much!

Would you be alright with me merging this as is and then adjusting the rules? I am currently in the process of migrating angular and there seem to be a lot of changes to the css that I didn't expect.

[blyedev]

I have nothing against that I can work asynchronously. I'll just file an issue to keep track of changes. What would you like to do about the test cases I haven't had the tome to fix yet? We could disable the linting check for now so I can keep working off a rebased branch without having to tweak the rules between multiple PR's

[johannesjo]

There will be another batch of bigger changes coming, if I do the migrations I planed (from scss to css variables and from @import to @use, which will be quite painful), so there likely will be a lot of conflicts, but fortunately I found a work-around so, it is not super urgent to do the changes, so I'll do that later then.

Thinking about the subject more, I came to the realization that ultimately I need to decide what rules are a good fit for the project. So it's not really an easy task to be delegated. It also doesn't make sense to do a lot of adjustments throughout the code base now, just to change it again a couple of commits later.

I should have thought about this earlier. I am sorry for only coming to this realization just now... :/

What would be super great, if you could prepare the basic setup, maybe – if this is possible – with all rules deactivated or at least all which are not a no-brainer and all that require a lot of changes throughout the code base, if that is possible. Then I can play around with the settings and enable the ones that fit one by one.

Or as an alternative route you could make a proposal what rules we should use and then we discuss it here? Generally I think, we should leave all formatting stuff to prettier (maybe we should use https://github.com/prettier/stylelint-prettier) and just to stick to the error avoidance cases for the linting.

Does that make sense to you? What do you think?

I agree that we should put this in the pre-commit hooks and use the autofix option if possible.

[blyedev]

I should have thought about this earlier. I am sorry for only coming to this realization just now... :/

Do not worry, I am happy to help out and do so partially out of selfish reasons, namely these are exactly the type of issues I'd like to experience since my projects do not have concerns like maintainability by 3rd parties and the scale yours has.

Thinking about the subject more, I came to the realization that ultimately I need to decide what rules are a good fit for the project. So it's not really an easy task to be delegated. It also doesn't make sense to do a lot of adjustments throughout the code base now, just to change it again a couple of commits later.

What would be super great, if you could prepare the basic setup, maybe – if this is possible – with all rules deactivated or at least all which are not a no-brainer and all that require a lot of changes throughout the code base, if that is possible. Then I can play around with the settings and enable the ones that fit one by one.

Since I recreated my PR I've slowly been realizing an incremental approach may be a better fit.

Stylelint has sharable configs that can extend other configs somewhat akin to inheritance. The current config that I proposed is stylelint-config-standard-scss. The rule for these configs is the recommended config enables error prevention and the standard config extends it by stylistic rules. Because stylelint inherently does not understand scss the scss variant of each exists.

We could downgrade the linting to the stylelint-config-recommended-scss which after disabling no-descending-specificity leaves us with 139 errors a lot of which seem to be actual errors.

For your consideration of maintainability I should note that while autofixes exist they do not seem to be as ubiquitous in scss as in css and out of the 139 only 9 are fixable. They seem to be far more common in the stylelistic errors which is to be expected given the fact they're easier and more important to fix automatically.

Now as per the final strictness of linting I would much prefer the standard config as personally I think it produces a much cleaner codebase. That however is an opinion of a person that enforces the order of selectors in their own projects so I will have to leave that decision up to you and your intuition of what's best for the project.

As per individual rules you'd like to enable/disable my suggestion is to use at least the recommended config as a base and we could choose which rules you'd like to disable. I have spent a lot of time reading about the stylelint ecosystem so I am more than willing to assist you in whatever way you deem fit. I think when using the recommended config as a base config a textual discussion of the rules in it is the best format and saves you the headache of a deep dive into it's configuration.

I propose I enable the recommended config and we discuss the failing rules and if you ever deem fit I could upgrade this to the standard config as is enabled in the PR right now.

---

As per the stylelistic rules and the stylelint-prettier plugin that you linked, I see no reason to make prettier a part of stylelint since you run it on all your files already and it would make no difference, given this is this plugins only functionality to my knowledge.

While changes like the leading space character may seem trivial they help enforce the recommended practices which are somewhat enforced anyways. If you de-commented such a line with your language server and re-commented it afterwards the space would be there automatically. In such a case the rule reduces unnecessary diff's similarly to what prettier does but many of the cases are css specific and not covered by prettier.

They are also a ton of work to maintain and enforce so I do not say they are absolutely necessary.

---

As a final note I should say, my involvement in this change is not limited to right now, if you feel the changes you're working on right now are a priority to this feel free to ping me when you're done.

[johannesjo]

Thanks for your understanding @blyedev !

I propose I enable the recommended config and we discuss the failing rules and if you ever deem fit I could upgrade this to the standard config as is enabled in the PR right now.

Yes, let's do it like this!

As per the stylelistic rules and the stylelint-prettier plugin that you linked, I see no reason to make prettier a part of stylelint since you run it on all your files already and it would make no difference, given this is this plugins only functionality to my knowledge.

I think it is less about making stylelint throwing errors on prettier stuff, than it is about making both work better with each other? Not sure though if this is actually the case.
Apparently this has been fixed:
https://github.com/prettier/stylelint-prettier?tab=readme-ov-file#disabling-rules-that-may-conflict-with-prettier

Normally I have no problem with strict rules. When working in teams, especially larger ones I even prefer it, but there are two things a bit special about this project:

1. Sometimes people contribute that are new to programming in general or to angular and frontend development in particular. It can be very frustrating and time consuming for these people when they have to deal with failing builds, when apparently nothing too important is wrong. When you are not familiar with the technologies, these things take much more time, than they might to an experienced frontend dev.
2. I like try out things fast sometimes. Due to angulars css files being mostly independent I haven't had much problems with that approach and it wouldn't make much sense to invest a lot of time in super tidy code, when it is possibly changed from ground up a while later.

This being said, I think we should go down the road you propose. And if some rules tend to get annoying while not offering much value, we kick them.

[blyedev]

Yes, let's do it like this!

Lovely, I'll start working on it and ping you once I review all currently failing rules

Apparently this has been fixed:
https://github.com/prettier/stylelint-prettier?tab=readme-ov-file#disabling-rules-that-may-conflict-with-prettier

Yes at the moment stylelint is not supposed to conflict with prettier. Funnily enough I'm pretty sure I found a bug according to that definition but I'll report it in the appropriate repo.

• Sometimes people contribute that are new to programming in general or to angular and frontend development in particular. It can be very frustrating and time consuming for these people when they have to deal with failing builds, when apparently nothing too important is wrong. When you are not familiar with the technologies, these things take much more time, than they might to an experienced frontend dev.

• I like try out things fast sometimes. Due to angulars css files being mostly independent I haven't had much problems with that approach and it wouldn't make much sense to invest a lot of time in super tidy code, when it is possibly changed from ground up a while later.

These are both great reasons I'll keep them in mind with my future recommendations.

[blyedev]

@johannesjo , I've set it up and did some tweaks for things I've noticed and most of the rules make sense to me right now.

This time I used an mjs file for the config so I can leave comments in the config for you and future contributors.

The vast majority of issues right now is duplicate selectors and declarations. I will merge the duplicate rules into one and delete declarations that are dead code. Question is do you have a preference as to the order of rules after they are merged? As in which one should I keep after i move all the code from the other one. I'm assuming I should just use my best judgement.

There a lot of empty blocks, would you rather I delete them or comment them out?

One rule that also popped up a lot is overriding shorthand properties via one of it's longhands. That one I realize could be debatable so I wonder if you think we should enforce it? I propose we merge any overriden shorthand with it's overriding declaration, makes the whole thing more explicit and less error prone.

By default the config disallows extensions in import statements, the rule allows you to also require them. I've never used scss before so I have no idea as to the implications of this but if it works we don't need to enforce this.

There's also these but I don't know what they do

src/_variables.scss
  63:38  ✖  Expected color.adjust($color, $alpha: -$amount) instead of              scss/no-global-function-names
            transparentize($color, $amount)
  64:31  ✖  Expected color.adjust($color, $alpha: -$amount) instead of              scss/no-global-function-names
            transparentize($color, $amount)

By default spacing around operators is also enforced:

src/styles/mixins/_theming.scss
  72:15  ✖  Expected single space before "+"  scss/operator-no-unspaced
  72:15  ✖  Expected single space after "+"   scss/operator-no-unspaced

I'm pretty sure that's it but we will only know once I resolve them.

[johannesjo]

Question is do you have a preference as to the order of rules after they are merged?

Not really :) I trust your judgement!

There a lot of empty blocks, would you rather I delete them or comment them out?

Let's delete them! But let's not add a rule for it if possible. It should be stripped away for the build anyway and within the context of the modular css it doesn't really hurt much imho.

One rule that also popped up a lot is overriding shorthand properties via one of it's longhands. That one I realize could be debatable so I wonder if you think we should enforce it?

I would prefer not to enforce it. I find something like

margin: 8px;
margin-right: 16px;
// vs margin: 8px 16px 8px 8px;

quite alright.

I've never used scss before so I have no idea as to the implications of this but if it works we don't need to enforce this.

Yes, I think so to!

[blyedev]

Hello again and I hope you're having a merry Christmas.

I have removed the duplicates. You've had a few !important's in there which I'm pretty sure stemmed exactly from the duplicates but in the spirit of making my changes only modify code not it's effect I have left them in there.

I have an update on the shorthand overrides, turns out I misunderstood the rule, it actually disallows overriding longhands via shorthands not the other way around. Modifying the code block you provided this would actually be the offending example:

margin-right: 16px;
margin: 8px;

Unless scss compiles this behaviour away somehow and differs from vanilla css I'm pretty sure that would make the line 1 dead code.

Other than that these are the only offenders left and I feel like most of them require your input due to my lack of familiarity with scss or the codebase:

src/_variables.scss
  63:38  ✖  Expected color.adjust($color, $alpha: -$amount) instead of              scss/no-global-function-names
            transparentize($color, $amount)
  64:31  ✖  Expected color.adjust($color, $alpha: -$amount) instead of              scss/no-global-function-names
            transparentize($color, $amount)
  69:39  ✖  Expected color.adjust($color, $alpha: -$amount) instead of              scss/no-global-function-names
            transparentize($color, $amount)
  70:32  ✖  Expected color.adjust($color, $alpha: -$amount) instead of              scss/no-global-function-names
            transparentize($color, $amount)

src/styles/page.scss
  16:26  ✖  Unexpected missing generic font family  font-family-no-missing-generic-family-keyword

src/styles/components/date-time-picker-schedule.scss
  783:18  ✖  Unexpected missing generic font family  font-family-no-missing-generic-family-keyword

src/styles/components/enlarge-img.scss
  71:3  ✖  Unexpected unknown property "user-drag"  property-no-unknown

src/styles/mixins/_material-icon.scss
  2:16  ✖  Unexpected missing generic font family  font-family-no-missing-generic-family-keyword

src/styles/mixins/_responsive-ratio.scss
  6:13  ✖  Unquote function used with an already-unquoted        scss/function-unquote-no-unquoted-strings-inside
           string
  6:13  ✖  Expected string.unquote instead of unquote            scss/no-global-function-names

[johannesjo]

Merry Christmas @blyedev ! 💫✨

Understood! Let's add this rule for now then. :)

scss/no-global-function-names will be a big migration unfortunately. We likely have to do it anyway at some point, since the next version of sass will require this. Up to you, if you feel like doing it!

scss/function-unquote-no-unquoted-strings-inside looks like a good rule. Afaics Unquote is not needed at this instance. The responsiveRatio mixing is not used anyway atm.

[blyedev]

scss/no-global-function-names will be a big migration unfortunately. We likely have to do it anyway at some point, since the next version of sass will require this. Up to you, if you feel like doing it!

I'm afraid that's beyond the scope of what I intended for this PR.

Either way all fails have been fixed and thus npm run lint:scss now exits without errors.

Is guess the only aspect that remains is if we integrate it into CI and precomit hooks now?

[johannesjo]

Alright! I am merging this then and add it to the pre-commit hook as well as the CI.

[blyedev]

Lovely! It's been a pleasure. Good luck with the project!

[johannesjo]

Just recognized that there are some logical changes to the code. As for shepherd.scss and inside util.scss. Did you test these?

[blyedev]

I did not explicitly test those but I'm pretty sure these are the merges of declarations, so they should not modify the logic. Give me a second I'll investigate further

[blyedev]

I'm not going to be able to explicitly test this myself for a second today but .shepherd-header has been merged. The diff's just look funky. It had 3 blocks with some duplicates among them so i removed the ones that would have had no effect such as the padding with no !important

Most of changes that could look like logical changes happened in this commit as that's the one in which i removed the duplicates

[johannesjo]

I just checked. The shepherd stuff seems to look fine to me :)

Thank you very much for your efforts!