-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
remove implicit style #4125
remove implicit style #4125
Conversation
Build Size ReportChanges to minified artifacts in 176 files changedTotal change +280 B View Changes
|
I can't tell what if anything you changed in the README, please revert all the unnecessary linting/spacing changes. The other changes for the base16 styles seem to be correct. I'm thinking they ship "complete" from the Base16 repo they are copied directly from to allow people to use them "as-is" from that repo... where-as the themes here are not meant to be used until they are "cooked" and added to a distribution. So I think this can be merged just as soon as we remove the unnecessary changes to the README. |
You mean in VERSION_11_UPGRADE.md (aebf8d0)? I removed all the unnecessary extra/double spaces.
You want me to add a new revert commit? Or do you prefer to drop this commit while squashing the PR? |
That'd be great. |
Build Size ReportChanges to minified artifacts in 174 files changedTotal change +284 B View Changes
|
Actually we may need more work here, but I'm not sure what to suggest. It just clicked that these are actually assets copied directly from another build system. So modifying them in our repo makes no sense - we'd need a script to make this repeatable. Otherwise all this work will be lost when the Base16 themes are rebuilt and copied over whenever we decide to refresh these in the future. I'm thinking it maybe best to leave these "as is" and clean up the small amount of duplication in our own build step. Thoughts? |
See last comment. Closing this. I feel like a tiny improvement to clean this up inside our CSS processing pipeline would be a different PR entirely - not a reuse of this one. |
Remove styles that are added automatically by the build system. cf. #4124 (comment)