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

Make CSS minifying compatible with calc() CSS function #9027

Merged
merged 12 commits into from Apr 19, 2017

Conversation

@sambolek
Copy link
Contributor

sambolek commented Mar 27, 2017

See #8552

Description

Whitespace around "-" sign got removed during CSS minification due to regex done in tubalmartin/cssmin, which got fixed in tubalmartin/YUI-CSS-compressor-PHP-port@59e5e8b#diff-80f11f7052c350cef68c3041a5479789R588

Fixed Issues (if relevant)

  1. #8552
  2. #6822

Manual testing scenarios

  1. Set Magento in production mode (minify doesn't work on developer or default modes)
  2. Extend Magento theme and use CSS calc() function in your less file. Example:
    body{width: calc((100% / 12 * 2) - 10px);}
  3. Turn Minify CSS on in admin
  4. Observe results, whitespace arond "-" sign should not be removed

Contribution checklist

  • Pull request has a meaningful description of its purpose
  • All commits are accompanied by meaningful commit messages
  • All new or changed code is covered with unit/integration tests (if applicable)
  • All automated tests passed successfully (all builds on Travis CI are green)
@magento-cicd2

This comment has been minimized.

Copy link
Contributor

magento-cicd2 commented Mar 27, 2017

CLA assistant check
All committers have signed the CLA.

@ishakhsuvarov

This comment has been minimized.

Copy link
Contributor

ishakhsuvarov commented Mar 29, 2017

Hi @sambolek
Please look into integration tests failure on Travis CI.
Thanks!

sambolek added 5 commits Mar 31, 2017
@sambolek

This comment has been minimized.

Copy link
Contributor Author

sambolek commented Mar 31, 2017

Fixed minification test. Also added a css class for scenario I mentioned in initial comment https://github.com/magento/magento2/pull/9027/files#diff-fc5a29319a5c4fcef6f8eb67024ebb20 which checks if calc is minified correctly.

@sambolek

This comment has been minimized.

Copy link
Contributor Author

sambolek commented Apr 1, 2017

Added test for issue #6822 , as this update would fix it as well.

@tubalmartin

This comment has been minimized.

Copy link

tubalmartin commented Apr 4, 2017

Just to let you know guys, as the maintainer of YUI-CSS-compressor-PHP-port, that I've just released v3.0.0 which is the most stable release to date. Updating to this version is strongly recommended.

@sambolek

This comment has been minimized.

Copy link
Contributor Author

sambolek commented Apr 4, 2017

Hi Túbal,
library updated to v3.0.0 - thanks for your good work :)

@vrann @ishakhsuvarov
Is this approach okay for replacing the library with a new psr-4 version, or should I implement a new css minify adapter and deprecate the current one?
d7266b3#diff-afc0f5b51cca601618e0ba6fd13ca75c

Will look into tests once they're run. For some reason they don't even start for some time, once I push a few commits in - is that intended behavior on Travis CI?

@ishakhsuvarov

This comment has been minimized.

Copy link
Contributor

ishakhsuvarov commented Apr 14, 2017

@sambolek Looks good to me, however we need to verify that nothing was broken by version bump.

@ishakhsuvarov

This comment has been minimized.

Copy link
Contributor

ishakhsuvarov commented Apr 14, 2017

@sambolek I'll try to run the full functional tests suite for starters, then we will continue with the rest, if you are ok with it :)

@sambolek

This comment has been minimized.

Copy link
Contributor Author

sambolek commented Apr 14, 2017

Sure.

@ishakhsuvarov ishakhsuvarov self-assigned this Apr 14, 2017
@ishakhsuvarov ishakhsuvarov added this to the April 2017 milestone Apr 14, 2017
@magento-team magento-team merged commit d7266b3 into magento:develop Apr 19, 2017
3 checks passed
3 checks passed
codacy/pr Good work! A positive pull request.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
licence/cla Contributor License Agreement is signed.
Details
magento-team pushed a commit that referenced this pull request Apr 19, 2017
@magento-team

This comment has been minimized.

Copy link
Contributor

magento-team commented Apr 19, 2017

@sambolek Thank you for the contribution

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.