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
Show file tree
Hide file tree
Changes from 10 commits
Commits
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
8 changes: 6 additions & 2 deletions .stylelintrc
Original file line number Diff line number Diff line change
@@ -1,10 +1,14 @@
{
"rules": {
"declaration-no-important": true
"declaration-no-important": true,
"csstree/validator": true
},
"processors": ["stylelint-processor-styled-components"],
"processors": ["stylelint-processor-styled-components", "./lib/stylelint/processorRemoveMixins.js"],
"extends": [
"stylelint-config-recommended",
"stylelint-config-styled-components"
],
"plugins": [
"stylelint-csstree-validator"
]
}
4 changes: 4 additions & 0 deletions lib/stylelint/processorRemoveMixins.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
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.

});
111 changes: 111 additions & 0 deletions lib/stylelint/processorRemoveMixins.test.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,111 @@
const processorRemoveMixins = require('./processorRemoveMixins');

it('should comment out styled-components mixin styles', () => {
const input = `
.selector78 {
list-style-type: none;
margin: $dummyValue 0 0 0;
-styled-mixin1: dummyValue;
-styled-mixin2: dummyValue;
padding: 0;
}
`;
const actual = processorRemoveMixins().code(input);
const expected = `
.selector78 {
list-style-type: none;
margin: $dummyValue 0 0 0;
/* styled-mixin */
/* styled-mixin */
padding: 0;
}
`;

expect(actual).toEqual(expected);
});

it('should comment out styled-components mixins styles in media queries', () => {
const input = `
.selector78 {
list-style-type: none;
margin: $dummyValue 0 0 0;
-styled-mixin1: dummyValue;
-styled-mixin2: dummyValue;
padding: 0;

@media (min-width: $dummyValue) {
margin: $dummyValue 0 0 0;
-styled-mixin3: dummyValue;
}
}
`;
const actual = processorRemoveMixins().code(input);
const expected = `
.selector78 {
list-style-type: none;
margin: $dummyValue 0 0 0;
/* styled-mixin */
/* styled-mixin */
padding: 0;

@media (min-width: $dummyValue) {
margin: $dummyValue 0 0 0;
/* styled-mixin */
}
}
`;

expect(actual).toEqual(expected);
});

it('should comment out styled-components mixins styles without semi-colons', () => {
const input = `
.selector78 {
list-style-type: none;
margin: $dummyValue 0 0 0;
-styled-mixin1: dummyValue;
-styled-mixin2: dummyValue;
-styled-mixin3: dummyValue
padding: 0;
-styled-mixin4: dummyValue
}
`;
const actual = processorRemoveMixins().code(input);
const expected = `
.selector78 {
list-style-type: none;
margin: $dummyValue 0 0 0;
/* styled-mixin */
/* styled-mixin */
/* styled-mixin */
padding: 0;
/* styled-mixin */
}
`;

expect(actual).toEqual(expected);
});

it('should not comment out styled-components mixins selectors', () => {
const input = `
-styled-mixin1:hover &,
-styled-mixin4:focus & {
-styled-mixin5: dummyValue
text-decoration: none;
border-bottom: $dummyValue solid $dummyValue;
-styled-mixin1: 0;
}
`;
const actual = processorRemoveMixins().code(input);
const expected = `
-styled-mixin1:hover &,
-styled-mixin4:focus & {
/* styled-mixin */
text-decoration: none;
border-bottom: $dummyValue solid $dummyValue;
/* styled-mixin */
}
`;

expect(actual).toEqual(expected);
});
27 changes: 27 additions & 0 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -85,6 +85,7 @@
"stylelint": "^10.1.0",
"stylelint-config-recommended": "^2.1.0",
"stylelint-config-styled-components": "^0.1.1",
"stylelint-csstree-validator": "^1.5.2",
"stylelint-processor-styled-components": "^1.8.0"
},
"husky": {
Expand Down
8 changes: 4 additions & 4 deletions packages/components/psammead-brand/src/index.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ const PADDING_AROUND_SVG_ABOVE_400PX = 56;
const PADDING_AROUND_SVG_BELOW_400PX = 32;

const conditionallyRenderHeight = (svgHeight, padding) =>
svgHeight ? `height: ${(svgHeight + padding) / 16}rem` : '';
svgHeight ? `height: ${(svgHeight + padding) / 16}rem;` : '';

const TRANSPARENT_BORDER = `0.0625rem solid transparent`;

Expand All @@ -32,13 +32,13 @@ const SvgWrapper = styled.div`
const Banner = styled.div`
background-color: ${C_POSTBOX};
${({ svgHeight }) =>
conditionallyRenderHeight(svgHeight, PADDING_AROUND_SVG_BELOW_400PX)};
conditionallyRenderHeight(svgHeight, PADDING_AROUND_SVG_BELOW_400PX)}
width: 100%;
padding: 0 ${GEL_SPACING};

@media (min-width: ${GEL_GROUP_2_SCREEN_WIDTH_MIN}) {
${({ svgHeight }) =>
conditionallyRenderHeight(svgHeight, PADDING_AROUND_SVG_ABOVE_400PX)};
conditionallyRenderHeight(svgHeight, PADDING_AROUND_SVG_ABOVE_400PX)}
padding: 0 ${GEL_SPACING_DBL};
}
border-top: ${({ borderTop }) => borderTop && TRANSPARENT_BORDER};
Expand Down Expand Up @@ -78,7 +78,7 @@ const BrandSvg = styled.svg`

/* stylelint-disable */
/* https://www.styled-components.com/docs/advanced#referring-to-other-components */
${StyledLink}:hover &,
${StyledLink}:hover &,
${StyledLink}:focus & {
text-decoration: none;
border-bottom: ${GEL_SPACING_HLF} solid ${C_WHITE};
Expand Down
2 changes: 1 addition & 1 deletion packages/components/psammead-navigation/src/index.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -101,7 +101,7 @@ const StyledListItem = styled.li`
`
: css`
right: 0;
`};
`}
}
`;

Expand Down
2 changes: 1 addition & 1 deletion packages/components/psammead-story-promo/src/index.jsx
Original file line number Diff line number Diff line change
Expand Up @@ -138,7 +138,7 @@ export const Headline = styled.h3`
}

return topStory ? getParagon(script) : getPica(script);
}};
}}

color: ${C_EBON};
${({ service }) => getSerifBold(service)}
Expand Down