build: add stylelint rule to ban concrete CSS rules inside theme files #10003
Conversation
googlebot
added
the
cla: yes
| // We use a regular `forEach`, instead of the PostCSS walker utils, | ||
| // because we only care about the top-level nodes. | ||
| root.nodes.forEach(node => { | ||
| if (node.type === 'rule') { |
There was a problem hiding this comment.
One note here @jelbourn: this will capture Sass placeholder rules (e.g. %something {}) which won't produce code unless they're extended. I opted to report them as well, because people can do things like %something, .something-else which does produce CSS.
There was a problem hiding this comment.
I'm on board with that. We generally don't use placeholders (I think just a couple in button). I'm a fan of the greater control mixins give you about what's being rendered.
| // We use a regular `forEach`, instead of the PostCSS walker utils, | ||
| // because we only care about the top-level nodes. | ||
| root.nodes.forEach(node => { | ||
| if (node.type === 'rule') { |
There was a problem hiding this comment.
I'm on board with that. We generally don't use placeholders (I think just a couple in button). I'm a fan of the greater control mixins give you about what's being rendered.
stylelint-config.json
Outdated
| "filePattern": "-theme\\.scss$" | ||
| }], | ||
| "material/no-concrete-rules": [true, { | ||
| "filePattern": "-theme\\.scss$" |
There was a problem hiding this comment.
I would make this any partial file (starts with an underscore).
| // We use a regular `forEach`, instead of the PostCSS walker utils, | ||
| // because we only care about the top-level nodes. | ||
| root.nodes.forEach(node => { | ||
| if (node.type === 'rule') { |
There was a problem hiding this comment.
Could this also catch doing @include some-mixin() in a partial?
There was a problem hiding this comment.
It could, but I'm not sure whether we want that. We have some cases where we use mixins inside the theme files to reduce duplication. We could make an exception for mixins that start with an underscore though.
There was a problem hiding this comment.
I mean specifically at the root level; @include inside of another mixin would be fine.
crisbeto
force-pushed
the
concrete-css-rules-rule
branch
2 times, most recently
from
c53de7e to
c697199
Compare
|
Addressed the feedback @jelbourn. Removing the "merge-safe" label, because I had to move some of the styling in the |
stylelint-config.json
Outdated
| "filePattern": "-theme\\.scss$" | ||
| }], | ||
| "material/no-concrete-rules": [true, { | ||
| "filePattern": "^_.*\\.scss$|-theme\\.scss$" |
There was a problem hiding this comment.
I wanted to capture cases like something-theme.scss, but I guess it's redundant since all the theme files are prefixed.
jelbourn
added
pr: lgtm
action: merge
Adds a stylelint rule that will ensure that all of the CSS rules inside theme files are placed inside a mixin. This avoids style duplication whenever the file is imported. Relates to angular#9999.
crisbeto
force-pushed
the
concrete-css-rules-rule
branch
from
c697199 to
ccba399
Compare
angular#10003) Adds a stylelint rule that will ensure that all of the CSS rules inside theme files are placed inside a mixin. This avoids style duplication whenever the file is imported. Relates to angular#9999.
angular#10003) Adds a stylelint rule that will ensure that all of the CSS rules inside theme files are placed inside a mixin. This avoids style duplication whenever the file is imported. Relates to angular#9999.
|
This issue has been automatically locked due to inactivity. Read more about our automatic conversation locking policy. This action has been performed automatically by a bot. |
3 participants
Adds a stylelint rule that will ensure that all of the CSS rules inside theme files are placed inside a mixin. This avoids style duplication whenever the file is imported.
Relates to #9999.