Add a CSS Validator

[dr3]

Is your feature request related to a problem? Please describe.
Highlighted by #2188, we should add a css validator to simorgh.

Having a look around, I noticed the package css-validator which we could run on the collated output of css from simorgh, however it currently has vulnerabilities (which we could fix twolfson/css-validator#15)

Alternatively we could introduce some form of styled components validator, ideally validating to the w3c spec, so look around and see if you can find a good one.

Describe the solution you'd like

• Carry out brief investigation of available options for validation that would catch issues like Invalid css - Padding auto #2188
• Add a validator that runs on CI/locally
• Briefly summarise reasons for going with option chosen
• Communicate change to rest of team

Testing notes
This issue will investigate css validators and then the check will be implemented on CI.Wouldn't require any testing from a tester.

Dev insight: This in itself would be a test, so limited if not no testing required, unless we write our own validator, which is likely inadvisable

Additional context
Add any other context or screenshots about the feature request here.

[sareh]

We already have this npm script test:lint:css that runs stylelint, but it isn't being used on CI. This ticket could be updated to be 'use stylelint linting in npm run test, since we run it on Husky pre-commit, so CSS linting errors can be picked up at this earlier stage.

[dr3]

@sareh I agree we should do that, as its currently failing on 1 thing, but not the issue in #2188, hence why i think we should add more

[sareh]

Agreed. When this ticket is picked up, it'd be good to explore usage of certain stylelint plugins , eg stylelint-csstree-validator.

[jroebu14]

@dr3 @sareh

I've been trying out stylelint-csstree-validator which catches things like padding: auto; however it doesn't like the use of mixins as shown below.

linting error when using mixins

---

example of invalid CSS and mixin usage

---

We could get around this by adding ignore rules to the stylelint config e.g.

    "csstree/validator": {
      "ignore": ["styled-mixin0"]
    }

however there can be multiple mixins within a file which would mean we would need to add an unknown number to the ignore section e.g. ["styled-mixin0", "styled-mixin1", "styled-mixin2", "styled-mixin3"]

[dr3]

@jroebu14 does it catch css that is programatically set? The padding auto from before was inside a function in the css like

padding: ${props.apple => props.apple ? 'auto' : 'foobar'}

[jroebu14]

I just discovered this https://www.styled-components.com/docs/tooling#interpolation-tagging

Looks like wherever we add mixins (or any sort of interpolation) we can tag it to let stylelint know what it is e.g.

export const Grid = styled.div`
  ${/* sc-custom */ layoutGridWrapper}
`;

That way it can pass stylelint validation.

does it catch css that is programatically set?

That's a very good question and the answer is, no it doesn't seem to 😞

[jroebu14]

@sareh @dr3

Here's an update on the use of the stylelint-csstree-validator plugin:

advantages

• Will catch invalid CSS that is static i.e. styles with no string interpolation (probably most of our CSS)

disadvantages

• we'd need to add tags when we use string interpolation in styles which might confuse some e.g.

export const Grid = styled.div`
  ${/* sc-custom */ layoutGridWrapper}
`;

• it doesn't catch CSS property value errors that are programatically set e.g. padding: ${({ apple }) => props.apple ? 'auto' : 'foobar'}

I don't think there are any alternatives out there that integrate well with styled-components.

To summarise, I think there's some value in adding stylelint-csstree-validator to our workflow even with the drawbacks mentioned above. I think most of our styles are not constructed of variables and mixins so there is potential for stylelint-csstree-validator to catch errors early.

Keen to hear your thoughts.

[dr3]

Interesting @jroebu14, I think i agree that it is worth adding, the string interpolation one is a bit of a pain though, as thats used soooooo much. Im assuming this would need to be used in psammead too?

I still think we need a validator on the rendered output of our pages, but this could be a nice step forward

[jroebu14]

@dr3 @sareh yeh we'd need to add this in Psammead too. OK cool so the plan to close this issue would be to open a PR each for psammead and simorgh with these changes:

1. Install stylelint-csstree-validator and configure in stylelintrc.
2. Fix linting new lint errors (probably mostly by tagging string interpolations).
3. Add npm run test:lint:css to npm run test script.
4. For psammead - bump changed packages

[jroebu14]

@dr3 @sareh

As I was working on implementing this in Simorgh and Psammead I noticed that using the sc-custom tag for our mixins was disabling linting all styles within the file.

---

Fig 1. csstree validator catching invalid property value but erroring on mixin

---

Fig 2. using styled-components interpolation tagging disables all linting

---

Fig 3. the only way to fix this is to disable and re-enable linting around the mixin which creates a lot of clutter in the styles.

---

If we want to continue using csstree/validator I suggest we go with one of these approaches to avoid cluttering styles with enable/disable comments

1. Add ignore rules to csstree/validator e.g.

  "rules": {
    "csstree/validator": {
      "ignore": ["-styled-mixin0", "-styled-mixin1", "-styled-mixin2", "-styled-mixin3", "-styled-mixin4", "-styled-mixin5"]
    }
  }

This would allow up to 5 mixins within a block of styles which should be sufficient as we generally don't go above 2 mixins per block.

2. Create a custom stylelint processor to run after the stylelint-processor-styled-components processor that removes -styled-mixin properties from styles.

3. Make a pull request to csstree/validator to accept regular expression ignore rules so that we only need to add 1 ignore rule in csstree/validator e.g.

  "rules": {
    "csstree/validator": {
      "ignore": ["/styled-mixin/"]
    }
  }

[dr3]

@jroebu14 Hmm thats a tricky one, for the time being can we raise an issue for 3, or maybe an issue for scoped ignore comments so

export const Grid = styled.div`
  // THIS IS CHECKED
  ${/* sc-custom-line */ layoutGridWrapper}
  // THIS IS CHECKED
`;

or even better

export const Grid = styled.div`
  // THIS IS CHECKED
  ${/* sc-custom-line styled-mixin */ layoutGridWrapper}
  // THIS IS CHECKED
`;

Perhaps? checking interest might make it easier to pick

[jroebu14]

Raised an issue for regular expression support csstree/stylelint-validator#19

[jroebu14]

@dr3

Based on this comment I quickly wrote a custom stylint processor to transform mixins into custom css properties.

BBC-archive/psammead#1184