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

WIP CSS: Don't automatically add "px" to properties with a few exceptions #4053

Closed
wants to merge 1 commit into from

Conversation

mgol
Copy link
Member

@mgol mgol commented Apr 23, 2018

Summary

Don't automatically add "px" to properties with a few exceptions

Fixes gh-2795
Ref gh-4009

Checklist

@mgol mgol added this to the 4.0.0 milestone Apr 23, 2018
@mgol
Copy link
Member Author

mgol commented Apr 23, 2018

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 margin or marginTop and I wasn't able to compress the full list in a regex or a short function so I included the whole list in the end.

@mgol
Copy link
Member Author

mgol commented Apr 23, 2018

This adds 120 43 bytes, quite a lot. :/ I'd appreciate ideas to reduce the size.


"use strict";

return {

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?

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?

Copy link
Member

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.

Copy link
Member Author

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.

Copy link
Member Author

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.

@mgol
Copy link
Member Author

mgol commented Apr 24, 2018

I've submitted an alternative version of this PR, see #4055.

@mgol mgol removed this from the 4.0.0 milestone Apr 30, 2018
@mgol mgol removed the Needs review label Apr 30, 2018
@mgol
Copy link
Member Author

mgol commented Apr 30, 2018

Closing this in favor of #4055.

@mgol mgol closed this Apr 30, 2018
@mgol mgol deleted the css-no-autopx branch April 30, 2018 16:53
@lock lock bot locked as resolved and limited conversation to collaborators Oct 27, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Development

Successfully merging this pull request may close these issues.

Stop appending px to everything except a blocklist?
3 participants