-
Notifications
You must be signed in to change notification settings - Fork 20.6k
WIP CSS: Don't automatically add "px" to properties with a few exceptions #4053
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
Conversation
This is WIP as it still needs more tests. @dmethvin I noticed my regex from https://gist.github.com/mgol/c14fcffb24cf9c2f08eca519f4dabd90 didn't catch a few things like |
This adds |
|
||
"use strict"; | ||
|
||
return { |
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.
As there's a lot of repetition here, would it be possible to build this using a combination of a few keywords?
Granted this file should probably be static, but: a handful of these are permutations of ["margin","border","padding"] + [...direction...] + ["", "width", "height"] and some form of population function for those combinations could happen?
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.
Additionally, as the constant "true" is used in all these cases-- could the number 1
be used instead to reduce space?
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.
Between Uglify and the zgip compression, many of these things go away. We don't generally care about the unminified size so things like true
to 1
don't help. It's hard to know for sure if it's smaller after min+gzip unless you do the permutations and see what happens.
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.
@gvanderest In addition to what @dmethvin said, in JS there's no way to quickly compute a list of all props with a certain prefix, infix & suffix; we'd need to add a triple for-loop and that adds size, not reduces it.
Instead of constructing a list you can create a regexp but the conditions here are irregular enough that the regex would still be quite large. That said, I worked on it a little more and I have an alternative regex-based approach that matches more than we need but still a finite number of values so maybe it's OK. See #4055.
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 updated the PR, there were way too many properties listed here. It's +43 bytes now.
I've submitted an alternative version of this PR, see #4055. |
Closing this in favor of #4055. |
Summary
Don't automatically add "px" to properties with a few exceptions
Fixes gh-2795
Ref gh-4009
Checklist