Merge pull request #72 from primer/mkt/additional-css-color-var-checks

Add additional CSS variable/color mode checks

README.md

@@ -29,6 +29,7 @@ Within your [stylelint config object](http://stylelint.io/user-guide/configurati
 - [primer/no-override](./plugins/#primerno-override): Prohibits custom styles that target Primer CSS selectors.
 - [primer/no-unused-vars](./plugins/#primerno-unused-vars): Warns about SCSS variables that are declared by not used in your local files.
 - [primer/no-undefined-vars](./plugins/#primerno-undefined-vars): Prohibits usage of undefined CSS variables.
+- [primer/no-scale-colors](./plugins/#primerno-scale-colors): Prohibits the use of [non-functional scale CSS variables](https://primer.style/css/support/color-system#color-palette)
 - [primer/colors](./plugins/#primercolors): Enforces the use of certain color variables.
 - [primer/spacing](./plugins/#primerspacing): Enforces the use of spacing variables for margin and padding.
 - [primer/typography](./plugins/#primertypography): Enforces the use of typography variables for certain CSS properties.

__tests__/__fixtures__/defines-new-color-vars.scss

@@ -0,0 +1,8 @@
+:root {
+  @include color-mode-var('my-first-feature', var(--color-scale-blue-5), var(--color-scale-blue-3));
+  @include color-mode-var(
+    'my-second-feature',
+    var(--color-scale-green-5),
+    var(--color-scale-red-3)
+  );
+}

__tests__/no-scale-colors.js

@@ -0,0 +1,33 @@
+const path = require('path')
+const {messages, ruleName} = require('../plugins/no-scale-colors')
+
+testRule({
+  plugins: ['./plugins/no-scale-colors.js'],
+  ruleName,
+  config: [
+    true,
+    {
+      files: [path.join(__dirname, '__fixtures__/color-vars.scss')]
+    }
+  ],
+
+  accept: [
+    {code: '.x { color: var(--color-text-primary); }'},
+    {code: '@include color-mode-var(my-feature, var(--color-scale-blue-1), var(--color-scale-blue-5))'}
+  ],
+
+  reject: [
+    {
+      code: '.x { color: var(--color-scale-blue-1); }',
+      message: messages.rejected('--color-scale-blue-1'),
+      line: 1,
+      column: 6
+    },
+    {
+      code: '.x { color: var(--color-auto-blue-1); }',
+      message: messages.rejected('--color-auto-blue-1'),
+      line: 1,
+      column: 6
+    }
+  ]
+})

__tests__/no-undefined-vars.js

@@ -9,6 +9,7 @@ testRule({
     {
       files: [
         path.join(__dirname, '__fixtures__/color-vars.scss'),
+        path.join(__dirname, '__fixtures__/defines-new-color-vars.scss'),
         path.join(__dirname, '__fixtures__/spacing-vars.scss')
       ]
     }
@@ -18,7 +19,10 @@ testRule({
     {code: '.x { color: var(--color-text-primary); }'},
     {code: '.x { color: var(--color-text-primary, #000000); }'},
     {code: '.x { background-color: var(--color-counter-bg); }'},
-    {code: '.x { margin: var(--spacing-spacer-1); }'}
+    {code: '.x { color: var(--color-my-first-feature); }'},
+    {code: '.x { color: var(--color-my-second-feature); }'},
+    {code: '.x { margin: var(--spacing-spacer-1); }'},
+    {code: '@include color-mode-var("feature", var(--color-scale-blue-1), var(--color-scale-blue-2))'}
   ],
 
   reject: [
@@ -33,6 +37,12 @@ testRule({
       message: messages.rejected('--color-bar'),
       line: 1,
       column: 6
+    },
+    {
+      code: '@include color-mode-var(feature, var(--color-scale-blue-1), var(--color-fake-2))',
+      message: messages.rejected('--color-fake-2'),
+      line: 1,
+      column: 1
     }
   ]
 })

package.json

@@ -1,6 +1,6 @@
 {
   "name": "stylelint-config-primer",
-  "version": "9.2.4",
+  "version": "9.3.0",
   "description": "Sharable stylelint config used by GitHub's CSS",
   "homepage": "http://primer.style/css/tools/linting",
   "author": "GitHub, Inc.",

plugins/README.md

@@ -7,6 +7,7 @@ This directory contains all of our custom stylelint plugins, each of which provi
 - [`primer/no-override`](#primerno-override)
 - [`primer/no-unused-vars`](#primerno-unused-vars)
 - [`primer/no-undefined-vars`](#primerno-undefined-vars)
+- [`primer/no-scale-colors`](#primerno-scale-colors)
 - [`primer/colors`](#primercolors)
 - [`primer/spacing`](#primerspacing)
 - [`primer/typography`](#primertypography)
@@ -120,6 +121,20 @@ Because there isn't a good way for a stylelint plugin to know what CSS variables
 - `files` is a single path, glob, or array of paths and globs, that tells the plugin which files (relative to the current working directory) to scan for CSS variable declarations. The default is `['**/*.scss', '!node_modules']`, which tells [globby] to find all the `.scss` files recursively and ignore the `node_modules` directory.
 - `verbose` is a boolean that enables chatty `console.warn()` messages telling you what the plugin found, which can aid in debugging more complicated project layouts.
 
+## `primer/no-scale-colors`
+
+This rule prohibits the use of [non-functional scale CSS variables](https://primer.style/css/support/color-system#color-palette) like `var(--color-scale-blue-1)` in all cases except the `color-mode-var` mixin.
+
+```scss
+// Okay; using scale colors while defining new variables
+@include color-scale-var('new-var-name', var(--color-scale-blue-1), var(--color-scale-blue-2))
+
+// Fail; using scale colors directly as a property value
+.selector {
+  color: var(--color-scale-blue-1)
+}
+```
+
 ## `primer/colors`
 
 This [variable rule](#variable-rules) enforces the use of Primer [color system](https://primer.style/css/support/color-system) variables for `color` and `background-color` CSS properties. Generally speaking, variables matching the pattern `$text-*` are acceptable for `color` (and `fill`), and `$bg-*` are acceptable for `background-color`. See [the configuration](./colors.js) for more info.

plugins/no-scale-colors.js

@@ -0,0 +1,54 @@
+const stylelint = require('stylelint')
+const matchAll = require('string.prototype.matchall')
+
+const ruleName = 'primer/no-scale-colors'
+const messages = stylelint.utils.ruleMessages(ruleName, {
+  rejected: varName =>
+    `${varName} is a non-functional scale color and cannot be used without being wrapped in the color-mode-var mixin`
+})
+
+// Match CSS variable references (e.g var(--color-text-primary))
+// eslint-disable-next-line no-useless-escape
+const variableReferenceRegex = /var\(([^\),]+)(,.*)?\)/g
+
+module.exports = stylelint.createPlugin(ruleName, (enabled, options = {}) => {
+  if (!enabled) {
+    return noop
+  }
+
+  const {verbose = true} = options
+  // eslint-disable-next-line no-console
+  const log = verbose ? (...args) => console.warn(...args) : noop
+
+  // Keep track of declarations we've already seen
+  const seen = new WeakMap()
+
+  return (root, result) => {
+    root.walkRules(rule => {
+      rule.walkDecls(decl => {
+        if (seen.has(decl)) {
+          return
+        } else {
+          seen.set(decl, true)
+        }
+
+        for (const [, variableName] of matchAll(decl.value, variableReferenceRegex)) {
+          log(`Found variable reference ${variableName}`)
+          if (variableName.match(/^--color-(scale|auto)-/)) {
+            stylelint.utils.report({
+              message: messages.rejected(variableName),
+              node: decl,
+              result,
+              ruleName
+            })
+          }
+        }
+      })
+    })
+  }
+})
+
+function noop() {}
+
+module.exports.ruleName = ruleName
+module.exports.messages = messages

plugins/no-undefined-vars.js

@@ -12,6 +12,9 @@ const messages = stylelint.utils.ruleMessages(ruleName, {
 // Match CSS variable definitions (e.g. --color-text-primary:)
 const variableDefinitionRegex = /(--[\w|-]*):/g
 
+// Match CSS variables defined with the color-mode-var mixin (e.g. @include color-mode-var(my-feature, ...))
+const colorModeVariableDefinitionRegex = /color-mode-var\s*\(\s*['"]?([^'",]+)['"]?/g
+
 // Match CSS variable references (e.g var(--color-text-primary))
 // eslint-disable-next-line no-useless-escape
 const variableReferenceRegex = /var\(([^\),]+)(,.*)?\)/g
@@ -30,6 +33,35 @@ module.exports = stylelint.createPlugin(ruleName, (enabled, options = {}) => {
   const seen = new WeakMap()
 
   return (root, result) => {
+    function checkVariable(variableName, node) {
+      if (!definedVariables.has(variableName)) {
+        stylelint.utils.report({
+          message: messages.rejected(variableName),
+          node,
+          result,
+          ruleName
+        })
+      }
+    }
+
+    root.walkAtRules(rule => {
+      if (rule.name === 'include' && rule.params.startsWith('color-mode-var')) {
+        const innerMatch = rule.params.match(/^color-mode-var\s*\(\s*(.*)\)\s*$/)
+        if (innerMatch.length !== 2) {
+          return
+        }
+
+        const [, params] = innerMatch
+        const [, lightValue, darkValue] = params.split(',').map(str => str.trim())
+
+        for (const v of [lightValue, darkValue]) {
+          for (const [, variableName] of matchAll(v, variableReferenceRegex)) {
+            checkVariable(variableName, rule)
+          }
+        }
+      }
+    })
+
     root.walkRules(rule => {
       rule.walkDecls(decl => {
         if (seen.has(decl)) {
@@ -39,14 +71,7 @@ module.exports = stylelint.createPlugin(ruleName, (enabled, options = {}) => {
         }
 
         for (const [, variableName] of matchAll(decl.value, variableReferenceRegex)) {
-          if (!definedVariables.has(variableName)) {
-            stylelint.utils.report({
-              message: messages.rejected(variableName),
-              node: decl,
-              result,
-              ruleName
-            })
-          }
+          checkVariable(variableName, decl)
         }
       })
     })
@@ -67,6 +92,10 @@ function getDefinedVariables(files, log) {
         log(`${variableName} defined in ${file}`)
         definedVariables.add(variableName)
       }
+      for (const [, variableName] of matchAll(css, colorModeVariableDefinitionRegex)) {
+        log(`--color-${variableName} defined via color-mode-var in ${file}`)
+        definedVariables.add(`--color-${variableName}`)
+      }
     }
 
     return definedVariables