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

New rule: new-color-css-vars #81

Merged
merged 19 commits into from
Oct 3, 2023
Merged

New rule: new-color-css-vars #81

merged 19 commits into from
Oct 3, 2023

Conversation

langermank
Copy link
Contributor

@langermank langermank commented Sep 27, 2023

Adds a new rule to update old color CSS variables with new variables, including a fallback value. This should only run on the sx prop as we have Stylelint in place to cover the styled syntax.

@changeset-bot
Copy link

changeset-bot bot commented Sep 27, 2023

🦋 Changeset detected

Latest commit: 821ef4d

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
eslint-plugin-primer-react Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

Copy link
Member

@joshblack joshblack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Just had a question

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))'
Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Contributor

@TylerJDev TylerJDev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love it 🤩!

One tiny comment, do we need a docs page for this, or nah? (e.g. no-deprecated-colors)

Copy link
Member

@broccolinisoup broccolinisoup left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! 🤩

I think we should add a documentation and include the rule in the README as well 🤗

@langermank
Copy link
Contributor Author

@broccolinisoup @TylerJDev going to merge this and add the docs in a separate PR!

@langermank langermank merged commit 35f0ffe into main Oct 3, 2023
4 checks passed
@primer-css primer-css mentioned this pull request Oct 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants