-
Notifications
You must be signed in to change notification settings - Fork 9
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
New rule: new-color-css-vars
#81
Merged
Merged
Changes from all commits
Commits
Show all changes
19 commits
Select commit
Hold shift + click to select a range
fc6d725
wip
colebemis b7a836d
new attempt
langermank e127f22
add tests
langermank a38be53
cleanup
langermank 6a0a63a
use sourceCode to check for variables
siddharthkp 0b852a4
fix syntax for last test
siddharthkp b2c3374
add another failing test case
siddharthkp c2774e5
upgrade eslint version to 8.42
siddharthkp 108b8b5
handle identifiers, add more test cases
siddharthkp bf51fb4
use css vars
langermank 4eb42d8
fix tests
langermank 1e4ade5
remove unused file
langermank 361f852
import rule
langermank bf08eea
export rules in the recommanded config
broccolinisoup eb2ad67
test to fix autofix
langermank a089bcd
try to fix autofix again
langermank f9eb90f
only support sx prop in favor of stylelint
langermank 2cc2ee7
Merge branch 'main' into new-css-vars
langermank 821ef4d
Create early-ads-clap.md
langermank File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
--- | ||
"eslint-plugin-primer-react": patch | ||
--- | ||
|
||
New rule: `new-color-css-vars` to find/replace legacy CSS color vars in sx prop |
Large diffs are not rendered by default.
Oops, something went wrong.
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,132 @@ | ||
const rule = require('../new-color-css-vars') | ||
const {RuleTester} = require('eslint') | ||
|
||
const ruleTester = new RuleTester({ | ||
parserOptions: { | ||
ecmaVersion: 'latest', | ||
sourceType: 'module', | ||
ecmaFeatures: { | ||
jsx: true | ||
} | ||
} | ||
}) | ||
|
||
ruleTester.run('no-color-css-vars', rule, { | ||
valid: [ | ||
{ | ||
code: `{color: 'fg.default'}` | ||
}, | ||
{ | ||
code: `<circle stroke="var(--color-border-default)" strokeWidth="2" />` | ||
}, | ||
{ | ||
code: `<circle fill="var(--color-border-default)" strokeWidth="2" />` | ||
}, | ||
{ | ||
code: `<div style={{ color: 'var(--color-border-default)' }}></div>` | ||
}, | ||
{ | ||
code: `<Blankslate border></Blankslate>` | ||
} | ||
], | ||
invalid: [ | ||
{ | ||
code: `<Button sx={{color: 'var(--color-fg-muted)'}}>Test</Button>`, | ||
output: `<Button sx={{color: 'var(--fgColor-muted, var(--color-fg-muted))'}}>Test</Button>`, | ||
errors: [ | ||
{ | ||
message: 'Replace var(--color-fg-muted) with var(--fgColor-muted, var(--color-fg-muted))' | ||
} | ||
] | ||
}, | ||
{ | ||
code: ` | ||
<Box sx={{ | ||
'&:hover [data-component="copy-link"] button, &:focus [data-component="copy-link"] button': { | ||
color: 'var(--color-accent-fg)' | ||
} | ||
}}> | ||
</Box>`, | ||
output: ` | ||
<Box sx={{ | ||
'&:hover [data-component="copy-link"] button, &:focus [data-component="copy-link"] button': { | ||
color: 'var(--fgColor-accent, var(--color-accent-fg))' | ||
} | ||
}}> | ||
</Box>`, | ||
errors: [ | ||
{ | ||
message: 'Replace var(--color-accent-fg) with var(--fgColor-accent, var(--color-accent-fg))' | ||
} | ||
] | ||
}, | ||
{ | ||
code: `<Box sx={{boxShadow: '0 0 0 2px var(--color-canvas-subtle)'}} />`, | ||
output: `<Box sx={{boxShadow: '0 0 0 2px var(--bgColor-muted, var(--color-canvas-subtle))'}} />`, | ||
errors: [ | ||
{ | ||
message: 'Replace var(--color-canvas-subtle) with var(--bgColor-muted, var(--color-canvas-subtle))' | ||
} | ||
] | ||
}, | ||
{ | ||
code: `<Box sx={{border: 'solid 2px var(--color-border-default)'}} />`, | ||
output: `<Box sx={{border: 'solid 2px var(--borderColor-default, var(--color-border-default))'}} />`, | ||
errors: [ | ||
{ | ||
message: 'Replace var(--color-border-default) with var(--borderColor-default, var(--color-border-default))' | ||
} | ||
] | ||
}, | ||
{ | ||
code: `<Box sx={{backgroundColor: 'var(--color-canvas-default)'}} />`, | ||
output: `<Box sx={{backgroundColor: 'var(--bgColor-default, var(--color-canvas-default))'}} />`, | ||
errors: [ | ||
{ | ||
message: 'Replace var(--color-canvas-default) with var(--bgColor-default, var(--color-canvas-default))' | ||
} | ||
] | ||
}, | ||
{ | ||
name: 'variable in scope', | ||
code: ` | ||
const baseStyles = { color: 'var(--color-fg-muted)' } | ||
export const Fixture = <Button sx={baseStyles}>Test</Button> | ||
`, | ||
output: ` | ||
const baseStyles = { color: 'var(--fgColor-muted, var(--color-fg-muted))' } | ||
export const Fixture = <Button sx={baseStyles}>Test</Button> | ||
`, | ||
errors: [ | ||
{ | ||
message: 'Replace var(--color-fg-muted) with var(--fgColor-muted, var(--color-fg-muted))' | ||
} | ||
] | ||
}, | ||
{ | ||
name: 'merge in sx', | ||
code: ` | ||
import {merge} from '@primer/react' | ||
export const Fixture = props => <Button sx={merge({color: 'var(--color-fg-muted)'}, props.sx)}>Test</Button> | ||
`, | ||
output: ` | ||
import {merge} from '@primer/react' | ||
export const Fixture = props => <Button sx={merge({color: 'var(--fgColor-muted, var(--color-fg-muted))'}, props.sx)}>Test</Button> | ||
`, | ||
errors: [ | ||
{ | ||
message: 'Replace var(--color-fg-muted) with var(--fgColor-muted, var(--color-fg-muted))' | ||
} | ||
] | ||
}, | ||
{ | ||
code: `<Box sx={{borderColor: 'var(--color-border-default)'}} />`, | ||
output: `<Box sx={{borderColor: 'var(--borderColor-default, var(--color-border-default))'}} />`, | ||
errors: [ | ||
{ | ||
message: 'Replace var(--color-border-default) with var(--borderColor-default, var(--color-border-default))' | ||
} | ||
] | ||
} | ||
] | ||
}) |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,106 @@ | ||
const cssVars = require('../utils/css-variable-map.json') | ||
|
||
module.exports = { | ||
meta: { | ||
type: 'suggestion', | ||
hasSuggestions: true, | ||
fixable: 'code', | ||
docs: { | ||
description: 'Upgrade legacy CSS variables to Primitives v8 in sx prop' | ||
}, | ||
schema: [ | ||
{ | ||
type: 'object', | ||
properties: { | ||
skipImportCheck: { | ||
type: 'boolean' | ||
}, | ||
checkAllStrings: { | ||
type: 'boolean' | ||
} | ||
}, | ||
additionalProperties: false | ||
} | ||
] | ||
}, | ||
/** @param {import('eslint').Rule.RuleContext} context */ | ||
create(context) { | ||
const styledSystemProps = [ | ||
'bg', | ||
'backgroundColor', | ||
'color', | ||
'borderColor', | ||
'borderTopColor', | ||
'borderRightColor', | ||
'borderBottomColor', | ||
'borderLeftColor', | ||
'border', | ||
'boxShadow', | ||
'caretColor' | ||
] | ||
|
||
return { | ||
/** @param {import('eslint').Rule.Node} node */ | ||
JSXAttribute(node) { | ||
if (node.name.name === 'sx') { | ||
if (node.value.expression.type === 'ObjectExpression') { | ||
// example: sx={{ color: 'fg.default' }} or sx={{ ':hover': {color: 'fg.default'} }} | ||
const rawText = context.sourceCode.getText(node.value) | ||
checkForVariables(node.value, rawText) | ||
} else if (node.value.expression.type === 'Identifier') { | ||
// example: sx={baseStyles} | ||
const variableScope = context.sourceCode.getScope(node.value.expression) | ||
const variable = variableScope.set.get(node.value.expression.name) | ||
|
||
// if variable is not defined in scope, give up (could be imported from different file) | ||
if (!variable) return | ||
|
||
const variableDeclarator = variable.identifiers[0].parent | ||
const rawText = context.sourceCode.getText(variableDeclarator) | ||
checkForVariables(variableDeclarator, rawText) | ||
} else { | ||
// worth a try! | ||
const rawText = context.sourceCode.getText(node.value) | ||
checkForVariables(node.value, rawText) | ||
} | ||
} else if ( | ||
styledSystemProps.includes(node.name.name) && | ||
node.value && | ||
node.value.type === 'Literal' && | ||
typeof node.value.value === 'string' | ||
) { | ||
checkForVariables(node.value, node.value.value) | ||
} | ||
} | ||
} | ||
|
||
function checkForVariables(node, rawText) { | ||
// performance optimisation: exit early | ||
if (!rawText.includes('var')) return | ||
|
||
Object.keys(cssVars).forEach(cssVar => { | ||
if (Array.isArray(cssVars[cssVar])) { | ||
cssVars[cssVar].forEach(cssVarObject => { | ||
const regex = new RegExp(`var\\(${cssVar}\\)`, 'g') | ||
if ( | ||
cssVarObject.props.some(prop => rawText.includes(prop)) && | ||
regex.test(rawText) && | ||
!rawText.includes(cssVarObject.replacement) | ||
) { | ||
const fixedString = rawText.replace(regex, `var(${cssVarObject.replacement}, var(${cssVar}))`) | ||
if (!rawText.includes(fixedString)) { | ||
context.report({ | ||
node, | ||
message: `Replace var(${cssVar}) with var(${cssVarObject.replacement}, var(${cssVar}))`, | ||
fix: function(fixer) { | ||
return fixer.replaceText(node, node.type === 'Literal' ? `"${fixedString}"` : fixedString) | ||
} | ||
}) | ||
} | ||
} | ||
}) | ||
} | ||
}) | ||
} | ||
} | ||
} |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will folks always need to include a fallback value or is this an interim step as we're moving over to the new system?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interim step! Once v8 is released and fully rolled out, we will refactor this to not include the fallback. We could also add some logic to remove the old fallbacks, but they won't really hurt anything if they stick around.