Skip to content

Commit 898c228

Browse files
committed
Allow overridden styles to re-order in generated CSS
We ran into a weird scenario, where if you passed the following two objects to css() in this order: ```js { '@media all and (min-width: 1128px)': {}, }, { '@media all and (min-width: 744px)': { backgroundColor: 'red', }, '@media all and (min-width: 1128px)': { backgroundColor: 'blue', }, } ``` you would normally expect it to produce a style that has the 744px min-width media query first. Unfortunately, because the first object already had that media query as an ordered key in its OrderedElements object, it maintained its original position. This caused the 1128px media query to be output first, which is unexpected and ended up causing a visual bug. To fix this, I modified OrderedElements to always move overridden keys to the end. This now behaves more like you would expect regular CSS to work. I needed to add a special case for string handlers, since they aren't actually overriding a style but rather replacing a property with a newly computed value.
1 parent 59084de commit 898c228

4 files changed

Lines changed: 71 additions & 2 deletions

File tree

src/generate.js

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -219,7 +219,12 @@ const runStringHandlers = (
219219
// handlers are very specialized and do complex things.
220220
declarations.set(
221221
key,
222-
stringHandlers[key](declarations.get(key), selectorHandlers)
222+
stringHandlers[key](declarations.get(key), selectorHandlers),
223+
224+
// Preserve order here, since we are really replacing an
225+
// unprocessed style with a processed style, not overriding an
226+
// earlier style
227+
true
223228
);
224229
}
225230
}

src/ordered-elements.js

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,9 +19,13 @@ export default class OrderedElements {
1919
}
2020
}
2121

22-
set(key /* : string */, value /* : any */) {
22+
set(key /* : string */, value /* : any */, preserveOrder /* : ?boolean */) {
2323
if (!this.elements.hasOwnProperty(key)) {
2424
this.keyOrder.push(key);
25+
} else if (!preserveOrder) {
26+
const index = this.keyOrder.indexOf(key);
27+
this.keyOrder.splice(index, 1);
28+
this.keyOrder.push(key);
2529
}
2630

2731
if (value == null) {

tests/generate_test.js

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -258,6 +258,34 @@ ${formatStyles(actual)}
258258
}`, defaultSelectorHandlers);
259259
});
260260

261+
it('orders overrides in the expected way', () => {
262+
assertCSS('.foo', [
263+
{
264+
"@media (min-width: 400px)": {
265+
padding: 10,
266+
}
267+
},
268+
{
269+
"@media (min-width: 200px)": {
270+
padding: 20,
271+
},
272+
"@media (min-width: 400px)": {
273+
padding: 30,
274+
}
275+
}
276+
], `
277+
@media (min-width: 200px){
278+
.foo{
279+
padding:20px !important;
280+
}
281+
}
282+
@media (min-width: 400px){
283+
.foo{
284+
padding:30px !important;
285+
}
286+
}`, defaultSelectorHandlers);
287+
});
288+
261289
it('supports custom string handlers', () => {
262290
assertCSS('.foo', [{
263291
fontFamily: ["Helvetica", "sans-serif"]

tests/ordered-elements_test.js

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -53,6 +53,38 @@ describe("OrderedElements", () => {
5353
}, elems);
5454
});
5555

56+
it('moves overridden elements to the end', () => {
57+
const elems = new OrderedElements();
58+
59+
elems.set("a", 1);
60+
elems.set("b", 1);
61+
elems.set("a", 2);
62+
63+
assert.deepEqual({
64+
elements: {
65+
b: 1,
66+
a: 2,
67+
},
68+
keyOrder: ["b", "a"],
69+
}, elems);
70+
});
71+
72+
it('can preserve order when setting', () => {
73+
const elems = new OrderedElements();
74+
75+
elems.set("a", 1);
76+
elems.set("b", 1);
77+
elems.set("a", 2, true);
78+
79+
assert.deepEqual({
80+
elements: {
81+
a: 2,
82+
b: 1,
83+
},
84+
keyOrder: ["a", "b"],
85+
}, elems);
86+
});
87+
5688
it("iterates over the elements in the correct order", () => {
5789
const elems = new OrderedElements();
5890

0 commit comments

Comments
 (0)