-
Notifications
You must be signed in to change notification settings - Fork 61
Fixes #37 #48
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
Fixes #37 #48
Conversation
} | ||
return to; | ||
}; | ||
} |
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.
We already have extend
for this type of thing, see
postcss-css-variables/index.js
Line 70 in ddb3b8b
var opts = extend({}, defaults, options); |
} | ||
|
||
.box-bar { | ||
width: var(--missing,calc(var(--foo1) + 100px)); |
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.
Let's space it out width: var(--missing, calc(var(--foo1) + 100px));
.box-foo { | ||
width: var(--missing); | ||
} | ||
|
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.
Why are we getting rid of this?
var fallbackValue; | ||
if(fallback) { | ||
var cloneDecl = Object.assign({},decl,{value:fallback}); | ||
fallbackValue = resolveValue(cloneDecl, map, false,true).value; |
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.
Let's space this out (sorry no eslint)
var cloneDecl = Object.assign({}, decl, { value: fallback });
fallbackValue = resolveValue(cloneDecl, map, false, true).value;
// Set the new value after we are done dealing with at-rule stuff | ||
decl.value = valueResults.value; | ||
} | ||
|
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.
Rebase this onto the current master
and squash things down. This is already in the codebase
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.
Some comments to address 😀
Thanks for the contribution. Closing in favor of #49 which seems to have the comments addressed and includes your commit. |
Fixes #37
Previous PR: #39