Review the rule differences from switching to stylelint

vanitabarrett commented 4 years ago

Part of https://github.com/alphagov/govuk-frontend/issues/1740

What

When Kevin raised the PR to switch to stylelint, he pointed out 5 rules which don't quite match up compared to what we currently have using sass-lint:

bem-depth - there isn't an equivalent core stylelint, there is a potential plugin route but doesn't appear to be like-for-like. Could potentially also be achieved with specifying class name regex.
border-zero - An equivalent rule doesn't exists in stylelint-core as it's rather divisive topic. We could apply this by a plugin but I'm inclined to not have a rule for it given the divisive aspects.
class-name-format - there isn't the same BEM option available in stylelint core. Instead gone for a relatively loose regexp that's roughly similar.
no-color-literals - there isn't an equivalent rule in stylelint core that forces colors to always be hex. Could consider doing something with a regex if this is a concern.
no-important - rather than allowing important values I left stylelint to raise an error on an important. This feels like the sort of rule developers should have to disable themselves for a single declaration.

We should review these differences and check that we're happy with the new implementation. Where something hasn't been implemented, we need to decide whether that particular rule is important to us and, if it is, see if there's an alternative solution.

Why

There are some differences between sass-lint and stylelint - we need to check that switching doesn't bring any unexpected changes that we aren't comfortable with.

vanitabarrett commented 4 years ago

@hannalaakso @36degrees

Ok to leave as-is

Rules I’m tempted to leave the way Kevin has implemented them.

border-zero An equivalent rule doesn't exists in stylelint-core as it's a rather divisive topic. We could apply this by a plugin but I'm inclined to not have a rule for it given the divisive aspects.

no-important rather than allowing important values I left stylelint to raise an error on an important. This feels like the sort of rule developers should have to disable themselves for a single declaration.

class-name-format there isn't the same BEM option available in stylelint core. Instead gone for a relatively loose regexp that's roughly similar.

Potential changes

Rules we could potentially change (either specifically for GOVUK Frontend or as suggested changes to stylelint-config-gds).

no-color-literals

There is no equivalent in stylelint, but we can use available rules to get some of that functionality. Kevin’s PR already handles the following:

Disallowing named colours (color-named: never)
Enforcing lowercase and long hex values (color-hex-length:long and color-hex-case: lower)

We could add hsl and hsla to the function-disallowed-list/ function-blacklist

You can preview the change here

bem_depth

There isn’t an equivalent in stylelint. A plugin does exist, but it doesn’t seem to do exactly what we need. Kevin’s PR already handles the following:

Disallow nesting over 2 items (max-nesting-depth: 2)

It’s possible we could expand the selector-class-pattern regex to get something equivalent to bem_depth, to stop selectors like this: .govuk-accordion__element__sub-element__sub-sub-element

You can preview the change here The regex can also be tested here: https://regexr.com/59o7e

36degrees commented 4 years ago

We could add hsl and hsla to the function-disallowed-list/ function-blacklist

Would this prevent us from using them in variable declarations? I think I'd be tempted to leave this change out. The intent of no-color-literals is to prevent you from using random colours within a ruleset. For example:

// Invalid
.foo {
    color: #ff0000; ❌
}

// Valid
$foo-text-color: #ff0000;

.foo {
    color: $foo-text-color; ✅
}

Blanket disallowing HSL / HSLA colours doesn't do the same thing, and whilst we might not use them at the minute I don't think it's something we need to enforce with linting rules.

It’s possible we could expand the selector-class-pattern regex to get something equivalent to bem_depth, to stop selectors like this: .govuk-accordion__element__sub-element__sub-sub-element

A little confused about this one as the current bem-depth rule appears to be disabled anyway? I think if we did enforce it then we even want to disallow e.g. .govuk-accordion__element__sub-element?

vanitabarrett commented 4 years ago

@36degrees

Would this prevent us from using them in variable declarations? I think I'd be tempted to leave this change out.

Yes, it would. I think because we're not using hsl/hsla at the moment I assumed it was something we recommended against using, but happy not to enforce that with linting.

A little confused about this one as the current bem-depth rule appears to be disabled anyway? I think if we did enforce it then we even want to disallow e.g. govuk-accordion__element__sub-element?

Good point, I think I got muddled there 🤦‍♀️ So I think we're ok with the nesting depth rule that's already been added in the PR and we don't want to enforce BEM depth? The regex could be adapted to disallow the rule you mention, but I guess if we're not enforcing it at the moment it makes sense to leave as-is for now and maybe revisit later?

hannalaakso commented 4 years ago

It's a shame that we can't replicate what no-color-literals used to do as it prevented the bloat of random colours. But overall I think this all looks okay 👍

vanitabarrett commented 4 years ago

@hannalaakso @36degrees Thanks both - I'm going to revert the WIP commits on Kevin's PR as we've decided they're not needed. There's also this PR that needs to be merged in and then Kevin's PR rebased to get the errors on use of !important https://github.com/alphagov/stylelint-config-gds/pull/4

alphagov / govuk-frontend