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

Place all rules with "all" declaration on top #479

Merged
merged 4 commits into from May 28, 2018

Conversation

Projects
None yet
6 participants
@chevsky
Contributor

chevsky commented May 28, 2018

Fix #458

@ai

This comment has been minimized.

Collaborator

ai commented May 28, 2018

@evilebottnawi

Good woks!

@coveralls

This comment has been minimized.

coveralls commented May 28, 2018

Coverage Status

Coverage increased (+0.001%) to 99.487% when pulling 7231632 on chevsky:fix-rules-containing-all into b6c64f2 on ben-eb:master.

@evilebottnawi

This comment has been minimized.

Member

evilebottnawi commented May 28, 2018

appveyor fails because we use npm@latest (https://github.com/ben-eb/cssnano/blob/master/appveyor.yml#L9 - it is npm@6) what is not compatibility with node@4, will be fixed in other PR 👍

@evilebottnawi evilebottnawi merged commit c133acb into cssnano:master May 28, 2018

3 of 4 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.001%) to 99.487%
Details
security/snyk No dependency changes
Details
@evilebottnawi

This comment has been minimized.

Member

evilebottnawi commented May 28, 2018

Thanks!

@ai

This comment has been minimized.

Collaborator

ai commented May 28, 2018

'should place rules with "all" at the top',
processCSS,
'.a{margin: 10px 20px;display: flex;all: initial;font-size: 10px;border-radius: 2px;color: blue;}.b{margin: 10px 30px;display: flex;all: initial;font-size: 20px;border-radius: 2px;color: blue;}',
'.a,.b{display: flex;all: initial;border-radius: 2px;color: blue;}.a{margin: 10px 20px;font-size: 10px;}.b{margin: 10px 30px;font-size: 20px;}'

This comment has been minimized.

@vxsx

vxsx May 29, 2018

these are not equivalent. before minification "a" doesn't have margin because all: initial overrides it, after minification it does have margin, see https://codepen.io/vxsx/pen/jKONOZ

This comment has been minimized.

@vxsx

vxsx May 29, 2018

you could argue that it's likely a mistake made by the css author, but css nano shouldn't fix that mistake for him

This comment has been minimized.

@chevsky

chevsky May 29, 2018

Contributor

@vxsx you are absolutely right. I guess the output should be like this:

.a {
  margin: 10px 20px;
}

.b{
  margin: 10px 30px;
}

.a, .b{
  display: flex;
  all: initial;
  border-radius: 2px;
}

.a{
  font-size: 10px;
  color: blue;
}

.b{
  font-size: 20px;
  color: red;
}

This is still shorter than the original if class names are not too long.
What do you think?

This comment has been minimized.

@evilebottnawi

evilebottnawi May 29, 2018

Member

@chevsky can you send PR? 👍

This comment has been minimized.

@chevsky

chevsky May 29, 2018

Contributor

Sure

This comment has been minimized.

@vxsx

This comment has been minimized.

@andyjansson

andyjansson Jul 14, 2018

Collaborator

This still appears to be broken.

\cc @evilebottnawi

This comment has been minimized.

@evilebottnawi

evilebottnawi Sep 20, 2018

Member

/cc @chevsky friendly ping

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment