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

Made stylelint throw errors if custom colours are found #3090

Merged
merged 14 commits into from May 9, 2019

Conversation

@mmmavis
Copy link
Member

commented May 2, 2019

Closes #2663

Stylelint will throw errors if rgb, rgba, hsl, hsla, hwb, gray, HTML color names are found outside of _variables.scss. This prevents us from accidentally introduce custom colour to the system.

Design team is in the process of eliminating use cases for using not 100% opaque colours. However, in cases where opacity modification is needed:

@cadecairos cadecairos temporarily deployed to foundation-mofostaging-pr-3090 May 2, 2019 Inactive

@mmmavis mmmavis temporarily deployed to foundation-mofostaging-pr-3090 May 2, 2019 Inactive

@mmmavis mmmavis requested a review from sabrinang May 2, 2019

@mmmavis mmmavis changed the title (WIP) Made stylelint throw errors if custom colours are found Made stylelint throw errors if custom colours are found May 2, 2019

@sabrinang
Copy link
Collaborator

left a comment

Reviewed with Mavis 👍 Looks good to me

@mmmavis mmmavis requested a review from Pomax May 2, 2019

@@ -30,6 +30,7 @@
"test:procfile": "node test/test-procfile.js",
"test:eslint": "eslint --config ./.eslintrc.json \"cypress/integration/*.js\" \"network-api/networkapi/{,!(frontend)/**/}*.js\" \"source/js/**/*.js\" \"source/js/**/*.jsx\" webpack.config.js",
"test:scss": "stylelint \"source/sass/**/*.scss\" \"source/js/**/*.scss\" --syntax scss",
"test:scss:styleguide": "stylelint \"source/sass/**/*.scss\" \"source/js/**/*.scss\" \"!source/sass/**/_variables.scss\" --syntax scss --config .stylelintrc-styleguide.json",

This comment has been minimized.

Copy link
@mmmavis

mmmavis May 2, 2019

Author Member

We want this task to only lint SCSS files outside of _variables.scss. Not super sure about task name though. Would test:scss:color be a better name?

This comment has been minimized.

Copy link
@Pomax

Pomax May 6, 2019

Collaborator

:styleguide is pretty good, it leaves the door open for other things like "only allowing font families we want" etc.

@alanmoo
Copy link
Member

left a comment

While I'm not going to fight this, I'm highly skeptical of restrictions like this being effective in any way, as I think this type of issue is a design problem, not an engineering one. If a new color comes in, an engineer that doesn't know the intent of this PR might simply add the new variable to the one file that they can, and continue on their way, rather than pushing back on it. Plus, making things like black and white into a variable feels...completely unnecessary.

That all being said, if there is insistence that we do this, we should do it right and make the rgba colors part of the variables rather than cheating and using a mixin.

@@ -5,7 +5,7 @@
right: 0;
bottom: 0;
left: 0;
background: rgba(255, 255, 255, 0.9);
background: transparentize($white, 0.1);

This comment has been minimized.

Copy link
@alanmoo

alanmoo May 3, 2019

Member

This feels like it violates the spirit of this PR.

@mmmavis

This comment has been minimized.

Copy link
Member Author

commented May 3, 2019

@alanmoo I agree that turning pure white and black into variables seems too much. Maybe we should make these two colours as exceptions. However, I think this kind of linting is really necessary. In the past designers and devs have struggled so much to keep design (especially colours) consistent across. Check out @sabrina's audit to get a sense of how many random colours there were on the site. Custom colours in codebase weren't always added by design. A lot of times new colours got added because design specs weren't detailed enough (or devs didn't have access to the Sketch file to inspect colours) that devs had to just guess the colour themselves (like grey, #ccc, #eee, #555, red, rgba(0, 0, 0, 0.3) to make a light gray & rgba(0, 0, 0, 0.5) to make a darker gray). This created issues as designers didn't have tools to verify colours on review apps & PR reviewers wouldn't spot this issue as PR had already passed design review.

I chatted with @sabrinang and it seems like our goal is to make sure we have zero custom colours and to try our best to stick with 100% opaque colours (@sabrinang please chime in in case I said something wrong). Designers are in the process of making these restrictions on Sketch so I'm hoping devs should work from the same restrictions as well.

If a new color comes in, an engineer that doesn't know the intent of this PR might simply add the new variable to the one file that they can, and continue on their way, rather than pushing back on it.

We should let devs on our team know going forward we wanna make sure we only use standardized colours. If they see colour in design that falls outside of the styleguide they should flag it to designers. I remember GitHub has the feature to set default reviewer for individual files. Will it help if we make sure by default a designer is set as PR reviewer when _variables.scss gets modified? This way we can ensure random colours don't "sneak in" just to pass linting errors.

@alanmoo

This comment has been minimized.

Copy link
Member

commented May 3, 2019

That sounds like a reasonable solution, perhaps partnered with a comment near the list of color variables talking about what's going on.

@@ -45,7 +45,7 @@
}

.col-12 {
border: 2px solid #dedede;
border: 2px solid $gray-20;

This comment has been minimized.

Copy link
@Pomax

Pomax May 6, 2019

Collaborator

Poor king Dedede =(

@@ -17,7 +17,7 @@
background: $white;
padding: 15px;
width: 100%;
box-shadow: 0 -1px 3px rgba(0, 0, 0, 0.15);
box-shadow: 0 -1px 3px transparentize($black, 0.85);

This comment has been minimized.

Copy link
@Pomax

Pomax May 6, 2019

Collaborator

Is this a real function, or did we come up with it? If the latter, can we rename it to something like transparent() instead?

This comment has been minimized.

Copy link
@mmmavis

mmmavis May 6, 2019

Author Member

it's SASS/SCSS' built-in function: https://sass-lang.com/documentation/functions/color#transparentize

note: its opposite function is called opacify

@@ -1,7 +1,7 @@
// product draft rules
.draft-product::before {
content: "This is a draft.";
color: #ffa7a7;
color: $light-red;

This comment has been minimized.

Copy link
@Pomax

Pomax May 6, 2019

Collaborator

Colour nit: unlike green or blue, there is no "light" red. It goes from red through a few named colours to pink, so this would be something either "salmon" or "rose" =)

This comment has been minimized.

Copy link
@mmmavis

mmmavis May 6, 2019

Author Member

I like current our color naming system 'cuz everything looks intuitive to me. However I'm not an expert in this area so if you'd like to propose a new naming convetion we should create a ticket and get designers to chime in. 🎨

This comment has been minimized.

Copy link
@Pomax

Pomax May 7, 2019

Collaborator

the scheme makes sense, it just leads to some color names that out of context don't make a lot of sense =)

@Pomax
Copy link
Collaborator

left a comment

Having read through the conversation, we should probably at least make the few colours that we "transparentize" dedicated named colors in _variables.scss that are "solid equivalents" of the transparent colours we use, in preparation for design saying what they should really be.

@mmmavis mmmavis temporarily deployed to foundation-mofostaging-pr-3090 May 6, 2019 Inactive

@mmmavis mmmavis temporarily deployed to foundation-mofostaging-pr-3090 May 6, 2019 Inactive

@mmmavis mmmavis temporarily deployed to foundation-mofostaging-pr-3090 May 6, 2019 Inactive

@mmmavis

This comment has been minimized.

Copy link
Member Author

commented May 6, 2019

@Pomax

at least make the few colours that we "transparentize" dedicated named colors in _variables.scss that are "solid equivalents" of the transparent colours we use, in preparation for design saying what they should really be.

Since we are adding this note to _variables.scss to make sure non-standardized styles don't sneak in there just to pass linting errors, we should probably create a follow-up ticket to handle what you suggested above as those styles are still to be reviewed and standardized by designers. Ideally we want this file to reflect what designers have in our shared Sketch style library so designers and devs can work from the same style guide.

image

@mmmavis

This comment has been minimized.

Copy link
Member Author

commented May 6, 2019

Maybe I should move those standardized variables to a new file and give it a more "styleguide" specific name?

@mmmavis mmmavis temporarily deployed to foundation-mofostaging-pr-3090 May 6, 2019 Inactive

/gray\(/i,
// uses of web color names are not allowed (with 'white' and 'black' being the only exceptions)
/[^\$-\w]lavender/i,
/^lavender/i,

This comment has been minimized.

Copy link
@mmmavis

mmmavis May 6, 2019

Author Member

to capture the instances when value of the CSS rule starts with a color name,

e.g.,

property: colorname; 
// we don't have to worry about the whitespace after `:` 
// as stylelint seems to trim beginning and tailing whitespace before it runs the checks

@mmmavis mmmavis temporarily deployed to foundation-mofostaging-pr-3090 May 6, 2019 Inactive

@mmmavis

This comment has been minimized.

Copy link
Member Author

commented May 6, 2019

Updating this PR atm. Will ping people for review again once I'm done!

@mmmavis mmmavis temporarily deployed to foundation-mofostaging-pr-3090 May 6, 2019 Inactive

@mmmavis mmmavis temporarily deployed to foundation-mofostaging-pr-3090 May 6, 2019 Inactive

/gray\(/i,
// uses of web color names are not allowed (with 'white' and 'black' being the only exceptions)
/[^\$-\w]lavender/i,
/^lavender/i,

This comment has been minimized.

Copy link
@mmmavis

mmmavis May 6, 2019

Author Member

this is to capture the instances when a rule's value start with <colorname>.

e.g.,

color: colorname;
// We don't have to worry about the space after `:`.
// stylelint trims out whitespaces around a rule's `value` before running the test
@mmmavis

This comment has been minimized.

Copy link
Member Author

commented May 6, 2019

@Pomax @alanmoo after changing things back and forth for a few times I've decided to move all color variables into its own new SCSS file _color.scss and make the stylelint color test run against all other SCSS files. Let me know what you think! 👀

Test:
image

@mmmavis mmmavis requested review from alanmoo and Pomax May 6, 2019

@mmmavis mmmavis temporarily deployed to foundation-mofostaging-pr-3090 May 7, 2019 Inactive

/gray\(/i,
// uses of web color names are not allowed (with 'white' and 'black' being the only exceptions)
/[^\$-\w](lavender|thistle|plum|violet|orchid|fuchsia|magenta|mediumorchid|mediumpurple|blueviolet|darkviolet|darkorchid|darkmagenta|purple|indigo|darkslateblue|slateblue|mediumslateblue|pink|lightpink|hotpink|deeppink|palevioletred|mediumvioletred|lightsalmon|salmon|darksalmon|lightcoral|indianred|crimson|firebrick|darkred|red|orangered|tomato|coral|darkorange|orange|yellow|lightyellow|lemonchiffon|lightgoldenrodyellow|papayawhip|moccasin|peachpuff|palegoldenrod|khaki|darkkhaki|gold|cornsilk|blanchedalmond|bisque|navajowhite|wheat|burlywood|tan|rosybrown|sandybrown|goldenrod|darkgoldenrod|per|chocolate|saddlebrown|sienna|brown|maroon|darkolivegreen|olive|olivedrab|yellowgreen|limegreen|lime|lawngreen|chartreuse|greenyellow|springgreen|mediumspringgreen|lightgreen|palegreen|darkseagreen|mediumseagreen|seagreen|forestgreen|green|darkgreen|mediumaquamarine|aqua|cyan|lightcyan|paleturquoise|aquamarine|turquoise|mediumturquoise|darkturquoise|lightseagreen|cadetblue|darkcyan|teal|lightsteelblue|powderblue|lightblue|skyblue|lightskyblue|deepskyblue|dodgerblue|cornflowerblue|steelblue|royalblue|blue|mediumblue|darkblue|navy|midnightblue|snow|honeydew|mintcream|azure|aliceblue|ghostwhite|whitesmoke|seashell|beige|oldlace|floralwhite|ivory|antiquewhite|linen|lavenderblush|mistyrose|gainsboro|lightgray|silver|darkgray|gray|dimgray|lightslategray|slategray|darkslategray)/i,
/^(lavender|thistle|plum|violet|orchid|fuchsia|magenta|mediumorchid|mediumpurple|blueviolet|darkviolet|darkorchid|darkmagenta|purple|indigo|darkslateblue|slateblue|mediumslateblue|pink|lightpink|hotpink|deeppink|palevioletred|mediumvioletred|lightsalmon|salmon|darksalmon|lightcoral|indianred|crimson|firebrick|darkred|red|orangered|tomato|coral|darkorange|orange|yellow|lightyellow|lemonchiffon|lightgoldenrodyellow|papayawhip|moccasin|peachpuff|palegoldenrod|khaki|darkkhaki|gold|cornsilk|blanchedalmond|bisque|navajowhite|wheat|burlywood|tan|rosybrown|sandybrown|goldenrod|darkgoldenrod|per|chocolate|saddlebrown|sienna|brown|maroon|darkolivegreen|olive|olivedrab|yellowgreen|limegreen|lime|lawngreen|chartreuse|greenyellow|springgreen|mediumspringgreen|lightgreen|palegreen|darkseagreen|mediumseagreen|seagreen|forestgreen|green|darkgreen|mediumaquamarine|aqua|cyan|lightcyan|paleturquoise|aquamarine|turquoise|mediumturquoise|darkturquoise|lightseagreen|cadetblue|darkcyan|teal|lightsteelblue|powderblue|lightblue|skyblue|lightskyblue|deepskyblue|dodgerblue|cornflowerblue|steelblue|royalblue|blue|mediumblue|darkblue|navy|midnightblue|snow|honeydew|mintcream|azure|aliceblue|ghostwhite|whitesmoke|seashell|beige|oldlace|floralwhite|ivory|antiquewhite|linen|lavenderblush|mistyrose|gainsboro|lightgray|silver|darkgray|gray|dimgray|lightslategray|slategray|darkslategray)/i

This comment has been minimized.

Copy link
@mmmavis

mmmavis May 7, 2019

Author Member

this is to capture the instances when a rule's value start with <colorname>.

e.g.,

color: colorname;
// We don't have to worry about the space after `:`.
// stylelint trims out whitespaces around a rule's `value` before running the test

This comment has been minimized.

Copy link
@Pomax

Pomax May 7, 2019

Collaborator

Rather than specifying this ourselves, could we use https://github.com/stylelint/stylelint/tree/master/lib/rules/color-named instead, and say never?

This comment has been minimized.

Copy link
@mmmavis

mmmavis May 7, 2019

Author Member

Yea that was my first attempt. But if we want to have black and white as exceptions then we can't use Stylelint's color-named 😭

This comment has been minimized.

Copy link
@Pomax

Pomax May 7, 2019

Collaborator

Do we, though? Using $black and $white seems pretty okay in everything except the variables file?

/gray\(/i,
// uses of web color names are not allowed (with 'white' and 'black' being the only exceptions)
/[^\$-\w](lavender|thistle|plum|violet|orchid|fuchsia|magenta|mediumorchid|mediumpurple|blueviolet|darkviolet|darkorchid|darkmagenta|purple|indigo|darkslateblue|slateblue|mediumslateblue|pink|lightpink|hotpink|deeppink|palevioletred|mediumvioletred|lightsalmon|salmon|darksalmon|lightcoral|indianred|crimson|firebrick|darkred|red|orangered|tomato|coral|darkorange|orange|yellow|lightyellow|lemonchiffon|lightgoldenrodyellow|papayawhip|moccasin|peachpuff|palegoldenrod|khaki|darkkhaki|gold|cornsilk|blanchedalmond|bisque|navajowhite|wheat|burlywood|tan|rosybrown|sandybrown|goldenrod|darkgoldenrod|per|chocolate|saddlebrown|sienna|brown|maroon|darkolivegreen|olive|olivedrab|yellowgreen|limegreen|lime|lawngreen|chartreuse|greenyellow|springgreen|mediumspringgreen|lightgreen|palegreen|darkseagreen|mediumseagreen|seagreen|forestgreen|green|darkgreen|mediumaquamarine|aqua|cyan|lightcyan|paleturquoise|aquamarine|turquoise|mediumturquoise|darkturquoise|lightseagreen|cadetblue|darkcyan|teal|lightsteelblue|powderblue|lightblue|skyblue|lightskyblue|deepskyblue|dodgerblue|cornflowerblue|steelblue|royalblue|blue|mediumblue|darkblue|navy|midnightblue|snow|honeydew|mintcream|azure|aliceblue|ghostwhite|whitesmoke|seashell|beige|oldlace|floralwhite|ivory|antiquewhite|linen|lavenderblush|mistyrose|gainsboro|lightgray|silver|darkgray|gray|dimgray|lightslategray|slategray|darkslategray)/i,
/^(lavender|thistle|plum|violet|orchid|fuchsia|magenta|mediumorchid|mediumpurple|blueviolet|darkviolet|darkorchid|darkmagenta|purple|indigo|darkslateblue|slateblue|mediumslateblue|pink|lightpink|hotpink|deeppink|palevioletred|mediumvioletred|lightsalmon|salmon|darksalmon|lightcoral|indianred|crimson|firebrick|darkred|red|orangered|tomato|coral|darkorange|orange|yellow|lightyellow|lemonchiffon|lightgoldenrodyellow|papayawhip|moccasin|peachpuff|palegoldenrod|khaki|darkkhaki|gold|cornsilk|blanchedalmond|bisque|navajowhite|wheat|burlywood|tan|rosybrown|sandybrown|goldenrod|darkgoldenrod|per|chocolate|saddlebrown|sienna|brown|maroon|darkolivegreen|olive|olivedrab|yellowgreen|limegreen|lime|lawngreen|chartreuse|greenyellow|springgreen|mediumspringgreen|lightgreen|palegreen|darkseagreen|mediumseagreen|seagreen|forestgreen|green|darkgreen|mediumaquamarine|aqua|cyan|lightcyan|paleturquoise|aquamarine|turquoise|mediumturquoise|darkturquoise|lightseagreen|cadetblue|darkcyan|teal|lightsteelblue|powderblue|lightblue|skyblue|lightskyblue|deepskyblue|dodgerblue|cornflowerblue|steelblue|royalblue|blue|mediumblue|darkblue|navy|midnightblue|snow|honeydew|mintcream|azure|aliceblue|ghostwhite|whitesmoke|seashell|beige|oldlace|floralwhite|ivory|antiquewhite|linen|lavenderblush|mistyrose|gainsboro|lightgray|silver|darkgray|gray|dimgray|lightslategray|slategray|darkslategray)/i

This comment has been minimized.

Copy link
@Pomax

Pomax May 7, 2019

Collaborator

Rather than specifying this ourselves, could we use https://github.com/stylelint/stylelint/tree/master/lib/rules/color-named instead, and say never?

@mmmavis mmmavis temporarily deployed to foundation-mofostaging-pr-3090 May 8, 2019 Inactive

@alanmoo

This comment has been minimized.

Copy link
Member

commented May 8, 2019

This seems fine but I'll let @Pomax give it the final stamp because he was working more closely with it.

@Pomax
Pomax approved these changes May 8, 2019
Copy link
Collaborator

left a comment

@mmmavis

This comment has been minimized.

Copy link
Member Author

commented May 9, 2019

Yea merging!

@mmmavis mmmavis merged commit a60daa9 into master May 9, 2019

4 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls First build on issue-2663-linter-raw-colour-code at 75.109%
Details
percy/foundation.mozilla.org Visual review approved by Mavis Ou
Details

@Pomax Pomax deleted the issue-2663-linter-raw-colour-code branch May 9, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.