Skip to content
This repository has been archived by the owner on Aug 13, 2023. It is now read-only.

CSS tree validator with custom processor #1184

Merged
merged 19 commits into from
Jul 22, 2019
Merged

CSS tree validator with custom processor #1184

merged 19 commits into from
Jul 22, 2019

Conversation

jroebu14
Copy link
Contributor

@jroebu14 jroebu14 commented Jul 12, 2019

Resolves bbc/simorgh#2189

Overall change: Adds the stylelint-csstree-validator plugin to catch invalid CSS properties and values.

A custom stylelint processor in the processors pipeline after stylelint-processor-styled-components comments out styled-components mixins so they don't fail stylelint-csstree-validator plugin validation.

Code changes:

  • Updates stylelint config to add custom style processor processorRemoveMixins
  • processorRemoveMixins uses a regex to match and replace any styled-components mixin with the CSS comment /* styled-mixin */ because CSS comments are ignored by stylelint
  • Fixes stylelint validation errors
  • Adds the test:lint:css script inside of the test script to prevent committing invalid CSS.
  • Includes unit tests for the custom processor
  • Includes comment about removing the custom processor if proposed functionality in stylelint-processor-styled-components is added

Testing notes
This doesn't require any testing because stylelint is developer dependency and doesn't modify any code used in production.

  • I have assigned myself to this PR and the corresponding issues
  • Tests added for new features
  • Test engineer approval

@jroebu14 jroebu14 changed the title WIP - css validator with custom processor CSS tree validator with custom processor Jul 15, 2019
@jroebu14 jroebu14 marked this pull request as ready for review July 15, 2019 11:03

module.exports = () => ({
code: input =>
input.replace(/-styled-mixin\d+:\s.+(?!{|,).$/gm, `/* styled-mixin */`),
Copy link
Contributor

Choose a reason for hiding this comment

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

Was there any particular reason/preference why we are commenting out rather than removing the styled-mixin?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeh there is. By removing the line with the styled-mixin it can open a can of worms of linting errors such as having a selector with no styles e.g.

const example = css`
  ${({ height }) => height ? `height: ${height};` : ''}
`;

would produce

.selector1 {
}

and also having a semi-colon where you don't need one e.g.

const example = css`
  width: 100%;
  ${({ height }) => height ? `height: ${height};` : ''}
`;

would produce

.selector1 {
  width: 100%; <--- semi colon not needed
}

Copy link
Contributor

Choose a reason for hiding this comment

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

The empty selector makes sense, that's not ideal. On the semi-colon is that suggesting the semi-colon's are only needed in CSS if there is a trailing statement?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that's true but after investigation I realised it's bad practice to do that and the semi-colon error must have been related to something else.

@@ -0,0 +1,7 @@
// If this proposed functionality ever gets added https://github.com/styled-components/stylelint-processor-styled-components/issues/271
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately it doesn't tell the CSS validator plugin to ignore those custom properties and rules. That's why the plugin has it's own ignore option which doesn't support regular expressions like stylelint's rules do. I have suggested this functionality in csstree/stylelint-validator

@PriyaKR
Copy link
Contributor

PriyaKR commented Jul 22, 2019

Doesn't require a testing from a tester.

@staylos92
Copy link
Contributor

@jroebu14 please update and merge this. Thanks

@staylos92 staylos92 merged commit ca2e974 into latest Jul 22, 2019
@staylos92 staylos92 deleted the add-css-validator branch July 22, 2019 14:01
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a CSS Validator
8 participants