Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 9 additions & 6 deletions src/generate.js
Original file line number Diff line number Diff line change
Expand Up @@ -177,7 +177,7 @@ export const generateCSS = (
// If none of the handlers handled it, add it to the list of plain
// style declarations.
if (!foundHandler) {
plainDeclarations.set(key, val);
plainDeclarations.set(key, val, true);
}
});

Expand All @@ -199,9 +199,9 @@ const runStringHandlers = (
declarations /* : OrderedElements */,
stringHandlers /* : StringHandlers */,
selectorHandlers /* : SelectorHandler[] */
) /* : OrderedElements */ => {
) /* : void */ => {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👍

if (!stringHandlers) {
return declarations;
return;
}

const stringHandlerKeys = Object.keys(stringHandlers);
Expand All @@ -219,12 +219,15 @@ const runStringHandlers = (
// handlers are very specialized and do complex things.
declarations.set(
key,
stringHandlers[key](declarations.get(key), selectorHandlers)
stringHandlers[key](declarations.get(key), selectorHandlers),

// Preserve order here, since we are really replacing an
// unprocessed style with a processed style, not overriding an
// earlier style
false
);
}
}

return declarations;
};


Expand Down
14 changes: 9 additions & 5 deletions src/ordered-elements.js
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,13 @@ export default class OrderedElements {
}
}

set(key /* : string */, value /* : any */) {
set(key /* : string */, value /* : any */, shouldReorder /* : ?boolean */) {
if (!this.elements.hasOwnProperty(key)) {
this.keyOrder.push(key);
} else if (shouldReorder) {
const index = this.keyOrder.indexOf(key);
this.keyOrder.splice(index, 1);
this.keyOrder.push(key);
}

if (value == null) {
Expand All @@ -36,7 +40,7 @@ export default class OrderedElements {
? this.elements[key]
: new OrderedElements();
value.forEach((value, key) => {
nested.set(key, value);
nested.set(key, value, shouldReorder);
});
this.elements[key] = nested;
return;
Expand All @@ -50,7 +54,7 @@ export default class OrderedElements {
: new OrderedElements();
const keys = Object.keys(value);
for (let i = 0; i < keys.length; i += 1) {
nested.set(keys[i], value[keys[i]]);
nested.set(keys[i], value[keys[i]], shouldReorder);
}
this.elements[key] = nested;
return;
Expand All @@ -70,12 +74,12 @@ export default class OrderedElements {
addStyleType(styleType /* : any */) /* : void */ {
if ((MAP_EXISTS && styleType instanceof Map) || styleType instanceof OrderedElements) {
styleType.forEach((value, key) => {
this.set(key, value);
this.set(key, value, true);
});
} else {
const keys = Object.keys(styleType);
for (let i = 0; i < keys.length; i++) {
this.set(keys[i], styleType[keys[i]]);
this.set(keys[i], styleType[keys[i]], true);
}
}
}
Expand Down
28 changes: 28 additions & 0 deletions tests/generate_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -258,6 +258,34 @@ ${formatStyles(actual)}
}`, defaultSelectorHandlers);
});

it('orders overrides in the expected way', () => {
assertCSS('.foo', [
{
"@media (min-width: 400px)": {
padding: 10,
}
},
{
"@media (min-width: 200px)": {
padding: 20,
},
"@media (min-width: 400px)": {
padding: 30,
}
}
], `
@media (min-width: 200px){
.foo{
padding:20px !important;
}
}
@media (min-width: 400px){
.foo{
padding:30px !important;
}
}`, defaultSelectorHandlers);
});

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

do we have a test to make sure string handlers still work correctly?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

There are some tests for string handlers. Is there a specific test you think I should add?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Just to make sure that string handlers don't end up at the end of style declarations. I'm less worried about this now that the argument is inverted, though. :)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Yep, there is a test that verifies that already. That's actually how I ended up making this an argument in the first place since I broke that test in my initial pass.

it('supports custom string handlers', () => {
assertCSS('.foo', [{
fontFamily: ["Helvetica", "sans-serif"]
Expand Down
32 changes: 32 additions & 0 deletions tests/ordered-elements_test.js
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,38 @@ describe("OrderedElements", () => {
}, elems);
});

it('preserves original order when overriding', () => {
const elems = new OrderedElements();

elems.set("a", 1);
elems.set("b", 1);
elems.set("a", 2);

assert.deepEqual({
elements: {
a: 2,
b: 1,
},
keyOrder: ["a", "b"],
}, elems);
});

it('can reorder when overriding', () => {
const elems = new OrderedElements();

elems.set("a", 1);
elems.set("b", 1);
elems.set("a", 2, true);

assert.deepEqual({
elements: {
b: 1,
a: 2,
},
keyOrder: ["b", "a"],
}, elems);
});

it("iterates over the elements in the correct order", () => {
const elems = new OrderedElements();

Expand Down