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

Fix inferring data type of border-[…] with multiple values #17248

Merged
merged 3 commits into from
Mar 17, 2025

Conversation

RobinMalfait
Copy link
Member

This PR fixes an issue where arbitrary values such as border-[12px_4px] were incorrectly producing border-color instead of border-width values.

To solve it, I extended the <line-width> check to make sure that every part of the value is a valid <line-width>.

In order for a line-width to be valid, every part should be one of:

  1. A keyword: thin, medium, thick
  2. A length: 12px
  3. A number: 0

Fixes: #17221

Test plan

  1. Added test to verify this works
  2. All existing tests pass

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature.

Verified

This commit was signed with the committer’s verified signature.
In order for a `line-width` to be valid, every part should be one of:

1. A keyword: `thin`, `medium`, `thick`
2. A length: `12px`
3. A number: `0`
@RobinMalfait RobinMalfait requested a review from a team as a code owner March 17, 2025 10:35
return segment(value, ' ').every(
(value) =>
isLength(value) ||
isNumber(value) ||
Copy link
Contributor

Choose a reason for hiding this comment

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

I think I know why isNumber is here… it's for 0 support right? If so we should really adjust isLength to account for unit-less zero instead I think.

Can do that in a separate PR tho.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah wasn't 100% sure where to put the unit-less numbers. Only annoying thing is that 0 can be put in spots where both a length or percentage is valid, so if we have a utility that differentiate between length and percentage then we run into an issue (and an explicit data type is necessary). Might not be a big deal 🤔

Also border-width: 1; is interpreted as border-width: 1px; but 0 stays as 0 (or at least by Lightning CSS). So we almost have to resolve the value based on the property we are setting: https://lightningcss.dev/playground/index.html#%7B%22minify%22%3Afalse%2C%22customMedia%22%3Atrue%2C%22cssModules%22%3Afalse%2C%22analyzeDependencies%22%3Afalse%2C%22targets%22%3A%7B%22chrome%22%3A6225920%7D%2C%22include%22%3A0%2C%22exclude%22%3A0%2C%22source%22%3A%22.foo%20%7B%5Cn%20%20border-width%3A%200%201%3B%20%2F*%200%201px%20*%2F%5Cn%20%20font-size%3A%201%3B%20%2F*%201px%20*%2F%5Cn%20%20--value%3A%201%3B%20%2F*%201%20*%2F%5Cn%7D%22%2C%22visitorEnabled%22%3Afalse%2C%22visitor%22%3A%22%7B%5Cn%20%20Color(color)%20%7B%5Cn%20%20%20%20if%20(color.type%20%3D%3D%3D%20'rgb')%20%7B%5Cn%20%20%20%20%20%20color.g%20%3D%200%3B%5Cn%20%20%20%20%20%20return%20color%3B%5Cn%20%20%20%20%7D%5Cn%20%20%7D%5Cn%7D%22%2C%22unusedSymbols%22%3A%5B%5D%2C%22version%22%3A%22local%22%7D

The inferring of data types is also not 100% correct conceptually because in this case it should be <line-width>{1,4} and now we passthrough any number of arguments.

@RobinMalfait RobinMalfait merged commit 4489493 into main Mar 17, 2025
5 checks passed
@RobinMalfait RobinMalfait deleted the fix/issue-17221 branch March 17, 2025 11:35
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.

length interpreted as color
2 participants