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

ENH Update stylelint rules #26

Merged

Conversation

GuySartorelli
Copy link
Member

After merging:

  1. create a new 1.3 branch
  2. tag 1.3.0 on GitHub
  3. Publish 1.3.0 to npm via yarn publish

Issue

@GuySartorelli GuySartorelli force-pushed the pulls/1/update-stylelint-rules branch from 23338a5 to f0e12f6 Compare June 5, 2024 03:36
'selector-class-pattern': null,
'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

@GuySartorelli GuySartorelli force-pushed the pulls/1/update-stylelint-rules branch 3 times, most recently from 3f436af to f857181 Compare June 5, 2024 23:29
Comment on lines -34 to +35
'selector-max-compound-selectors': 5,
'selector-max-compound-selectors': 6,
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.

Comment on lines 45 to +46
'max-nesting-depth': [
3,
4,
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

'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.

Comment on lines +125 to +127
'selector-id-pattern': null,
'keyframes-name-pattern': null,
'scss/dollar-variable-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.

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"

Comment on lines +128 to +129
'scss/at-extend-no-missing-placeholder': null,
'no-descending-specificity': 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.

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.

@emteknetnz emteknetnz merged commit 5b93480 into silverstripe:1 Jun 11, 2024
@emteknetnz emteknetnz deleted the pulls/1/update-stylelint-rules branch June 11, 2024 04:26
@GuySartorelli
Copy link
Member Author

Tagged and published as 1.3.0

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.

2 participants