Conversation
🦋 Changeset detectedLatest commit: 6bd3013 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
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 |
| '^color$': ['fgColor', 'iconColor'], | ||
| '^background(-color)?$': ['bgColor'], | ||
| '^border(-top|-right|-bottom|-left|-inline|-block)*(-color)?$': ['borderColor'], | ||
| '^background(-color|-image)?$': ['bgColor'], |
There was a problem hiding this comment.
Also test background-image
| '^background(-color)?$': ['bgColor'], | ||
| '^border(-top|-right|-bottom|-left|-inline|-block)*(-color)?$': ['borderColor'], | ||
| '^background(-color|-image)?$': ['bgColor'], | ||
| '^border(?:-top|-right|-bottom|-left|-inline|-block)?(?:-color)?$': ['borderColor'], |
There was a problem hiding this comment.
Enable testing for border
| (/^border(-top|-right|-bottom|-left|-inline|-block)*$/.test(prop) || /^background$/.test(prop)) && | ||
| !valueNode.value.toLowerCase().includes('color') | ||
| (/^border(?:-top|-right|-bottom|-left|-inline|-block)?$/.test(prop) || /^background$/.test(prop)) && | ||
| ['color', 'scale'].every(word => !valueNode.value.toLowerCase().includes(word)) |
There was a problem hiding this comment.
We now only return if neither color nor scale is in the token, because we want --display-blue-scale-0 to fail.
If this is risky we could also consider changing the name of the token to --display-blue-color-0 or --display-blue-colorScale-0 CC:@langermank
There was a problem hiding this comment.
Pull Request Overview
This PR enhances the color validation rule to handle background-image properties, exclude display-scale tokens from shorthand props, and adds corresponding test coverage.
- Broaden regexes to include
-imagefor backgrounds and refine border shorthand matching - Update shorthand logic to skip values containing “scale” as well as “color”
- Add new tests for rejecting
--display-*-scale-*variables in various properties
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| plugins/colors.js | Extended background regex, refined border shorthand logic, and updated the skip‐value check to also exclude “scale.” |
| tests/colors.js | Added tests to ensure display‐scale tokens are rejected by the rule |
| .changeset/pink-beds-mix.md | Bumped minor version and noted new background/border behavior |
Comments suppressed due to low confidence (3)
plugins/colors.js:139
- Typo in comment:
shortandshould beshorthand.
// Property is shortand and value doesn't include color
plugins/colors.js:29
- The updated border regex only allows one segment (e.g.,
-top) but drops support for multi-part shorthands likeborder-inline-block. Change?back to*on the segment group to restore repeated segments.
'^border(?:-top|-right|-bottom|-left|-inline|-block)?(?:-color)?$': ['borderColor'],
plugins/colors.js:51
- Same issue here: this
propTyperegex restricts to a single segment. Use(?:-top|-right|-bottom|-left|-inline|-block)*instead of?to allow multiple segments in shorthand border props.
} else if (/^border(?:-top|-right|-bottom|-left|-inline|-block)?(?:-color)?$/.test(prop)) {
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
jonrohan
left a comment
jonrohan
left a comment
There was a problem hiding this comment.
Seems ok, we should verify there isn't too many new errors on the dotcom codebase by running lint with the canary build
|
@jonrohan can you do this? I have no idea how to do what you just said. |
|
Hey @jonrohan did you check against dotcom? Do you think we are ready to merge? |
|
@lukasoppermann run an integration test against |
This pull request introduces updates to the color validation logic and test cases in the
colorsplugin. The changes primarily enhance the handling ofbackground,border, and related properties, expand the validation for shorthand properties, and add new test cases to cover display scale tokens.@langermank @jonrohan I hope this change make sense. It is the first time for me working in this repo. I'll add some comments in the code to explain