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
Changes from 1 commit
7c680af
dc7f15e
659f9ae
7294f27
e7677bb
4ca4a70
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -154,3 +154,38 @@ test( | |
'h1{margin:10px 20px 30px 40px}', | ||
'h1{margin:10px 20px 30px 40px}', | ||
); | ||
|
||
test( | ||
'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}' | ||
); | ||
|
||
test( | ||
'should merge rules with custom props', | ||
processCss, | ||
'h1{padding-top:var(--variable);padding-right:var(--variable);padding-bottom:var(--variable);padding-left:var(--variable)}', | ||
'h1{padding:var(--variable)}' | ||
); | ||
|
||
test( | ||
'should not break unmergeable fallbacks with custom props (2)', | ||
processCss, | ||
'h1{padding:10em;padding:var(--variable)}', | ||
'h1{padding:var(--variable);padding:10em}' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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..? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As described in issue #448 As far i understand that exactly what fix should do. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, but the correct order should be:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @karapetyan yep, we should have There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @andyjansson @evilebottnawi understood, thanks for review :) I’ll fix it soon. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @evilebottnawi friendly ping |
||
); | ||
|
||
test( | ||
'should not break unmergeable rules with custom props', | ||
processCss, | ||
'h1{padding:var(--variable)}', | ||
'h1{padding:var(--variable)}' | ||
); | ||
|
||
test( | ||
'should not break unmergeable fallbacks with custom props', | ||
processCss, | ||
'h1{padding:var(--some);padding:10em}', | ||
'h1{padding:var(--some);padding:10em}' | ||
); |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,11 +1,13 @@ | ||
import hasVariable from './hasVariable'; | ||
|
||
const important = node => node.important; | ||
const unimportant = node => !node.important; | ||
const hasInherit = node => ~node.value.indexOf('inherit'); | ||
const hasInitial = node => ~node.value.indexOf('initial'); | ||
|
||
export default (...props) => { | ||
if (props.some(hasInherit) || props.some(hasInitial)) { | ||
return props.every(hasInherit) || props.every(hasInitial); | ||
if (props.some(hasInherit) || props.some(hasInitial) || props.some(hasVariable)) { | ||
return props.every(hasInherit) || props.every(hasInitial) || props.every(hasVariable); | ||
} | ||
return props.every(unimportant) || props.every(important); | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,3 +1,30 @@ | ||
export default (rule, prop) => { | ||
return rule.filter(n => n.prop && ~n.prop.indexOf(prop)).pop(); | ||
}; | ||
function getRulesWithCustomProps (props, properties) { | ||
return properties.map(property => | ||
props.filter(n => n.prop && ~n.prop.indexOf(property) && n.value && ~n.value.search(/var/i)).pop() | ||
).filter(Boolean); | ||
} | ||
|
||
function getRulesWithoutCustomProps (props, properties) { | ||
return properties.map(property => | ||
props.filter(n => n.prop && ~n.prop.indexOf(property) && n.value && !~n.value.search(/var/i)).pop() | ||
).filter(Boolean); | ||
} | ||
|
||
export function ruleContainsCustomPropertiesAndFallbacks (props, properties) { | ||
return getRulesWithCustomProps(props, properties).length === getRulesWithoutCustomProps(props, properties).length; | ||
} | ||
|
||
export function getAllRules (props, properties) { | ||
return new Array( | ||
properties.map(property => | ||
props.filter(n => n.prop && ~n.prop.indexOf(property)).pop() | ||
).filter(Boolean) | ||
); | ||
} | ||
|
||
export function getRulesWithAndWithoutCustomProps (props, properties) { | ||
return new Array( | ||
getRulesWithoutCustomProps(props, properties), | ||
getRulesWithCustomProps(props, properties) | ||
); | ||
} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Looks we should rename this file to |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,7 +1,7 @@ | ||
import getLastNode from './getLastNode'; | ||
import {ruleContainsCustomPropertiesAndFallbacks, getRulesWithAndWithoutCustomProps, getAllRules} from './getLastNode'; | ||
|
||
export default function getRules (props, properties) { | ||
return properties.map(property => { | ||
return getLastNode(props, property); | ||
}).filter(Boolean); | ||
return ruleContainsCustomPropertiesAndFallbacks(props, properties) ? | ||
getRulesWithAndWithoutCustomProps(props, properties) : | ||
getAllRules(props, properties); | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,2 @@ | ||
export default node => | ||
~node.value.search(/var/i); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't it be:
?
There was a problem hiding this comment.
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) :
so I guess this is correct behavior
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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!