-
-
Notifications
You must be signed in to change notification settings - Fork 320
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: units being removed in math functions #894
Conversation
Please fix the CI, else LGTM |
lowerCasedValue === 'url' || | ||
lowerCasedValue === 'min' || | ||
lowerCasedValue === 'max' || | ||
lowerCasedValue === 'clamp' |
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.
We need to look on CSS spec for all possible values
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.
min()
, max()
and clamp()
were introduced in CSS Values Module Level 4, although I don't know if there are other CSS functions in other modules that don't accept unitless 0
.
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.
Hm, what about calc?
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.
I've had a look and units on 0 values are preserved already in calc
— maybe these should be moved to the same place? This would mean the same value conversions that happen in calc
would happen in these Level 4 functions.
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.
It is strange, I think logic for calc should be here too
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.
I think that would introduce a lot of regressions in the current transformations in calc
.
How about something like this to bring the functions together:
if (
lowerCasedValue === 'calc' ||
+ lowerCasedValue === 'min' ||
+ lowerCasedValue === 'max' ||
+ lowerCasedValue === 'clamp' ||
lowerCasedValue === 'hsl' ||
lowerCasedValue === 'hsla'
) {
walk(node.nodes, (n) => {
if (n.type === 'word') {
parseWord(n, opts, true);
}
});
return false;
}
- if (
- lowerCasedValue === 'url' ||
- lowerCasedValue === 'min' ||
- lowerCasedValue === 'max' ||
- lowerCasedValue === 'clamp'
- ) {
+ if (lowerCasedValue === 'url') {
return false;
}
}
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.
Yes, let's do it
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.
New pull request?
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.
Yep
Most of the failing CI seems to be disconnected from the changes I made, so not sure if it is in the scope of this pull request to fix them? |
Codecov Report
@@ Coverage Diff @@
## master #894 +/- ##
=======================================
Coverage 97.27% 97.28%
=======================================
Files 118 118
Lines 3455 3459 +4
Branches 1037 1041 +4
=======================================
+ Hits 3361 3365 +4
Misses 86 86
Partials 8 8
Continue to review full report at Codecov.
|
jest timeout has to be increased .. I guess 30s might be enough. |
Would it be appropriate to do that in this PR? |
Yes |
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.
LGTM
I couldn't find any other function, let me know if there is any !
Thanks! |
I believe this is an important fix. I've just spend 3 hours trying to figure this one out in a project. It is not a well-known fact fact a unitless 0's mess with function such as Meanwhile, a hack would be to use a very low value (eg. 0.01px) or just "0px" via a custom properties. But those are just hacks. |
Would it be possible to backport this fix into any 4.x.x release available via npm? At the moment there is no possibility to fix this while working on a react-script project without upgrading the whole webpack subsystem and even yarn resolutions can't force commit for this exact subpackage. |
When using
min()
,max()
orclamp()
,0
values get their units stripped, making the value invalid. This pull request preserves any units within these CSS functions.