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.

Sign up for GitHub

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

Jump to bottom

Media query option to preserver order #92

Closed
wants to merge 2 commits into from
Closed

Media query option to preserver order #92

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d0564ec
#30 - added option to preserve at-rule order
erikthalen Jan 14, 2019
103127e
removed console.log
erikthalen Jan 14, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 5 additions & 2 deletions index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,7 +65,10 @@ var defaults = {
variables: {},
// Preserve variables injected via JS with the `variables` option above
// before serializing to CSS (`false` will remove these variables from output)
preserveInjectedVariables: true
preserveInjectedVariables: true,
// Will write media queries in the same order as in the original file.
// Set it to true if you're working with min-width
preserveAtRulesOrder: false
};

module.exports = postcss.plugin('postcss-css-variables', function(options) {
Expand Down Expand Up @@ -249,7 +252,7 @@ module.exports = postcss.plugin('postcss-css-variables', function(options) {
ruleToWorkOn.nodes.slice(0).forEach(function(node) {
if(node.type === 'decl') {
var decl = node;
resolveDecl(decl, map, opts.preserve, logResolveValueResult);
resolveDecl(decl, map, opts.preserve, logResolveValueResult, opts.preserveAtRulesOrder);
}
});
});
Expand Down
18 changes: 13 additions & 5 deletions lib/resolve-decl.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -71,8 +71,9 @@ function eachMapItemDependencyOfDecl(variablesUsedList, map, decl, cb) {

// Resolve the decl with the computed value
// Also add in any media queries that change the value as necessary
function resolveDecl(decl, map, /*optional*/shouldPreserve, /*optional*/logResolveValueResult) {
function resolveDecl(decl, map, /*optional*/shouldPreserve, /*optional*/logResolveValueResult, /*optional*/preserveAtRulesOrder) {
shouldPreserve = shouldPreserve || false;
preserveAtRulesOrder = preserveAtRulesOrder || false;
Copy link

Choose a reason for hiding this comment

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

I believe the expected behaviour would be to maintain the CSS rule order. Therefore, would this make more sense defaulting to true?

Copy link
Owner

Choose a reason for hiding this comment

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

@awilson82-vp I am in favor of making the default be in order as expected. We can make a major release to play nice for people who have already worked around it.

Even removing the option completely seems reasonable

Copy link

Choose a reason for hiding this comment

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

@MadLittleMods Could we release this new option as-is now (minor release), and then change default to true in the next major release? That would allow us to start using it immediately.
Happy to help out if anything else needs to be done before this can be released.

Copy link
Owner

Choose a reason for hiding this comment

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

@dampkwab We can do a major release now (no need to wait for anything else). We just need to update the default in this PR.

A minor release is just extra work for no reason.

Copy link

Choose a reason for hiding this comment

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

@MadLittleMods I've updated the default now - hope that helps :)
#100

Copy link
Owner

Choose a reason for hiding this comment

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

#100 (comment) is now abandoned

I created #101 which removes the playground/ changes so we can build fresh from master. It seems like we should have some more tests though. So someone will need to pick that up

If we updated this merge request default to true, all of the current tests would need to be updated which would give us nice coverage.

Copy link

Choose a reason for hiding this comment

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

@MadLittleMods
Uff, I really need this fix ASAP =D
What about sticking with harmless preserveAtRulesOrder=false as default until you finish your wished result ^^)

Copy link
Owner

Choose a reason for hiding this comment

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

@dinodujmovic Sure 🤷‍♀ , now published: #92 (comment)

Copy link

Choose a reason for hiding this comment

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

Thanks a lot !


// Make it chainable
var _logResolveValueResult = function(valueResults) {
Expand All @@ -93,6 +94,7 @@ function resolveDecl(decl, map, /*optional*/shouldPreserve, /*optional*/logResol
// Resolve the cascade dependencies
// Now find any at-rule declarations that need to be added below each rule
//console.log('resolveDecl 2');
var previousAtRuleNode;
eachMapItemDependencyOfDecl(valueResults.variablesUsed, map, decl, function(mimicDecl, mapItem) {
var ruleClone = shallowCloneNode(decl.parent);
var declClone = decl.clone();
Expand All @@ -112,6 +114,7 @@ function resolveDecl(decl, map, /*optional*/shouldPreserve, /*optional*/logResol

// Add the rule to the atRule
atRuleNode.append(ruleClone);
//console.log(atRuleNode)


// Since that atRuleNode can be nested in other atRules, we need to make the appropriate structure
Expand All @@ -129,14 +132,19 @@ function resolveDecl(decl, map, /*optional*/shouldPreserve, /*optional*/logResol
currentAtRuleNode = currentAtRuleNode.parent;
}

// Put the atRuleStructure after the declaration's rule
decl.parent.parent.insertAfter(decl.parent, parentAtRuleNode);
// Put the first atRuleStructure after the declaration's rule,
// and after that, put them right after the previous one
decl.parent.parent.insertAfter(preserveAtRulesOrder && previousAtRuleNode || decl.parent, parentAtRuleNode);

// Save referance of previous atRuleStructure
previousAtRuleNode = parentAtRuleNode
}
else {
ruleClone.selector = mimicDecl.parent.selector;

// Put the atRuleStructure after the declaration's rule
decl.parent.parent.insertAfter(decl.parent, ruleClone);
// Put the first atRuleStructure after the declaration's rule,
// and after that, put them right after the previous one
decl.parent.parent.insertAfter(preserveAtRulesOrder && previousAtRuleNode || decl.parent, ruleClone);
}
});

Expand Down