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.

Sign up for GitHub

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

ENH Update stylelint rules #26

Merged
merged 1 commit into from
Jun 11, 2024
+39 −7
Merged

ENH Update stylelint rules #26

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
44 changes: 38 additions & 6 deletions .stylelintrc.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
module.exports = {
'defaultSeverity': 'warning',
'extends': [
'stylelint-config-recommended-scss',
'stylelint-config-sass-guidelines',
Expand All @@ -8,6 +7,8 @@ module.exports = {
'plugins': [
'stylelint-scss'
],
// Ignore anything that isn't one of these file types
'ignoreFiles': ['**/!(*.{css,scss,sass})'],
'rules': {
// Line Spacing
'rule-empty-line-before': [
Expand All @@ -31,7 +32,7 @@ module.exports = {
}
],
'selector-max-id': 2,
'selector-max-compound-selectors': 5,
'selector-max-compound-selectors': 6,
Comment on lines -34 to +35
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bumped slightly to resolve a lot of issues and avoid having to add a ignore comments for all of them.
This rule can't be retrospectively respected without changing the specificity of selectors, which might affect styling in adverse ways.

'selector-pseudo-element-colon-notation': 'single',
'no-descending-specificity': [
true,
Expand All @@ -42,7 +43,7 @@ module.exports = {
}
],
'max-nesting-depth': [
3,
4,
Comment on lines 45 to +46
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bumped slightly to resolve a lot of issues and avoid having to add a ignore comments for all of them.
Respecting this rule retrospectively would require refactoring the scss in a way that could result in unexpected changes

{
'ignore': [
'blockless-at-rules',
Expand Down Expand Up @@ -73,7 +74,31 @@ module.exports = {
]
}
],

'at-rule-no-unknown': [
true,
{
// Allow sass at-rules - see https://sass-lang.com/documentation/at-rules/
'ignoreAtRules': [
'at-root',
'debug',
'each',
'else',
'error',
'extend',
'for',
'forward',
'function',
'if',
'import',
'include',
'mixin',
'return',
'use',
'warn',
'while',
]
}
],
'comment-no-empty': true,
'declaration-block-no-duplicate-properties': true,
'no-irregular-whitespace': true,
Expand All @@ -86,14 +111,21 @@ module.exports = {
}
],
'font-family-name-quotes': 'always-unless-keyword',
// Turn off rules
// Turn off rules
'selector-class-pattern': null,
'selector-id-pattern': null,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can't change what the IDs look like at this point, it's too likely to cause problems.

'function-no-unknown': null,
'property-no-vendor-prefix': null,
'value-no-vendor-prefix': null,
'font-family-no-missing-generic-family-keyword': null,
'scss/dollar-variable-colon-space-after': null,
'scss/no-global-function-names': null,
'value-keyword-case': null
'value-keyword-case': null,
'media-query-no-invalid': null,
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As per https://stylelint.io/user-guide/rules/media-query-no-invalid/ this rule isn't appropriate for scss

'selector-id-pattern': null,
'keyframes-name-pattern': null,
'scss/dollar-variable-pattern': null,
Comment on lines +125 to +127
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A mixture of "changing these could cause problems" and "changing these isn't worth the effort, it doesn't really matter if they match some arbitrary pattern or not"

'scss/at-extend-no-missing-placeholder': null,
'no-descending-specificity': null,
Comment on lines +128 to +129
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no-descending-specificity was the single most common item failing linting, and respecting it retrospectively could result in unexpected changes in styling.

scss/at-extend-no-missing-placeholder means we can't use @extend to avoid duplication of code for class selectors - in some cases this would even mean duplicating bootstrap css which we definitely don't want to do. Best to just disable the rule.

}
};
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "@silverstripe/eslint-config",
"version": "1.2.1",
"version": "1.3.0",
"description": "SilverStripe config files for eslint",
"engines": {
"node": ">=18.x"
Expand Down