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

fix ben-eb/cssnano/448 don't remove fallbacks for custom css properties #486

Merged
merged 6 commits into from Jun 15, 2018
Merged

fix ben-eb/cssnano/448 don't remove fallbacks for custom css properties #486

merged 6 commits into from Jun 15, 2018

Conversation

karapetyan
Copy link
Contributor

@karapetyan karapetyan commented Jun 2, 2018

@ai could you check, please.
Thanks.

fixes #448

@ai
Copy link
Member

ai commented Jun 2, 2018

/cc @evilebottnawi

@coveralls
Copy link

coveralls commented Jun 2, 2018

Coverage Status

Coverage decreased (-0.03%) to 99.454% when pulling 659f9ae on karapetyan:save-fallbacks-for-custom-props into ce52717 on ben-eb:master.

'should not break unmergeable fallbacks with custom props (2)',
processCss,
'h1{padding:10em;padding:var(--variable)}',
'h1{padding:var(--variable);padding:10em}'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks wrong. Surely the output should be the same as the input..?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As described in issue #448
cssnano shouldn't remove fallbacks for custom CSS properties.

As far i understand that exactly what fix should do.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but the correct order should be:

h1{padding:10em;padding:var(--variable)}

var(--variable) wouldn't be understood by browsers not supporting it, thus falling back on 10em. In your version, you overwrite var(--variable) with 10em in all cases.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@karapetyan yep, we should have h1{padding:10em;padding:var(--variable)} here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@andyjansson @evilebottnawi understood, thanks for review :) I’ll fix it soon.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@evilebottnawi friendly ping

'should merge custom props and dont remove fallbacks',
processCss,
'h1{padding-top:10px;padding-right:15px;padding-bottom:20px;padding-left:25px;padding-top:var(--variable);padding-right:var(--variable);padding-bottom:var(--variable);padding-left:var(--variable)}',
'h1{padding:var(--variable);padding:10px 15px 20px 25px}'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't it be:

 h1{padding:10px 15px 20px 25px;padding:var(--variable)}

?

Copy link
Contributor Author

@karapetyan karapetyan Jun 4, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we look at many other tests that contain more than one unmegable rule, they have similar behavior.

Here is some examples from same test file (postcss-merge-longhand.js) :

test(
    'should not merge padding values with mixed !important',
    processCss,
    'h1{padding-top:10px!important;padding-right:20px;padding-bottom:30px!important;padding-left:40px}',
    'h1{padding-bottom:30px!important;padding-left:40px;padding-right:20px;padding-top:10px!important}',
);

test(
    'should not merge identical border values with mixed !important',
    processCss,
    'h1{border-top:1px solid #000;border-bottom:1px solid #000;border-left:1px solid #000!important;border-right:1px solid #000!important}',
    'h1{border-bottom:1px solid #000;border-left:1px solid #000!important;border-right:1px solid #000!important;border-top:1px solid #000}',
);

so I guess this is correct behavior

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I disagree, the semantics have fundamentally changed. See my other comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@andyjansson done, thanks for help!

getRulesWithoutCustomProps(props, properties),
getRulesWithCustomProps(props, properties)
);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks we should rename this file to utils.js

@andyjansson
Copy link
Contributor

Can you add a test where the properties are mixed up to demonstrate its correctness? E.g.:

h1{
  padding-top:10px;
  padding-right:var(--variable);
  padding-right:15px;
  padding-bottom:var(--variable);
  padding-bottom:20px;
  padding-left:25px;
  padding-top:var(--variable);
  padding-left:var(--variable)
}

@karapetyan
Copy link
Contributor Author

@andyjansson done

@andyjansson
Copy link
Contributor

@karapetyan okay, looking at the final test, I don't think the implementation is correct. padding-right and padding-bottom should be overwritten by concrete values, while the rest should not.

@alexander-akait
Copy link
Member

@andyjansson can you explain what tests we should add more?

@andyjansson
Copy link
Contributor

The correct minification should be something like:

h1{
  padding: 10px 15px 20px 25px;
  padding-left:var(--variable)
  padding-top:var(--variable);
}

@karapetyan
Copy link
Contributor Author

@andyjansson previous implementation was overcoded and awful, thank you for review. What you think about new one?

Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very good work, let's wait @andyjansson review 👍

@andyjansson
Copy link
Contributor

I'll be off this weekend, so I won't be able to properly review this until Monday. But at first glance, it seems good.

@alexander-akait
Copy link
Member

alexander-akait commented Jun 8, 2018

@andyjansson 👍 i will close other problems for cssnano@4 and Monday after review we can merge this and release rc.3, when two-three week look cssnano@rc.3in wild and release stable 4 version

@andyjansson
Copy link
Contributor

Alright, the implementation looks good. I was a bit confused by the odd ordering of the properties in the tests until I realised I was looking at integration tests (the reordering is due to css-declaration-sorter).

I'd like some unit tests where we test the plugin in isolation as well, if you don't mind (under /packages/postcss-merge-longhand/src/__tests__). The other thing that caught my eye was isCustomProp which could possibly be written so that false-positives are less likely to happen.

Apart from that, it looks really good. Good job!

@karapetyan
Copy link
Contributor Author

@andyjansson @evilebottnawi great review, thank you guys. It was review driven development, actually)

@@ -0,0 +1,2 @@
export default node =>
~node.value.search(/var(\s+\(|\()(\s+--|--)/i);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this could be simplified a bit. E.g.:

~node.value.search(/var\s*\(\s*--/i);

Last nitpick, I promise!

@alexander-akait
Copy link
Member

/cc @andyjansson

@alexander-akait alexander-akait merged commit 348dcfa into cssnano:master Jun 15, 2018
@alexander-akait
Copy link
Member

@karapetyan Thanks!

@YozhikM
Copy link
Contributor

YozhikM commented Jun 15, 2018

/home/stanislav/projects/cssnano/packages/postcss-merge-longhand/src/__tests__/borders.js
  316:5  error  'processCss' is not defined  no-undef
  323:5  error  'processCss' is not defined  no-undef
  330:5  error  'processCss' is not defined  no-undef
/home/stanislav/projects/cssnano/packages/postcss-merge-longhand/src/__tests__/columns.js
  75:5  error  'processCss' is not defined  no-undef

Change it to processCSS instead

@YozhikM
Copy link
Contributor

YozhikM commented Jun 15, 2018

@karapetyan @evilebottnawi
I actually came for someone else, I think it's necessary to fix it, but not in my PR

@alexander-akait
Copy link
Member

@YozhikM PR welcome 👍

YozhikM added a commit to YozhikM/cssnano that referenced this pull request Jun 15, 2018
@YozhikM YozhikM mentioned this pull request Jun 15, 2018
andyjansson pushed a commit that referenced this pull request Jun 15, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

mergeLonghand is not always a safe transformation
6 participants