Allow overridden styles to re-order in generated CSS by lencioni · Pull Request #279 · Khan/aphrodite

lencioni · 2017-10-11T22:58:16Z

We ran into a weird scenario, where if you passed the following two
objects to css() in this order:

{
  '@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.

cc @DanielGarcia-Carrillo

This method works by mutating its first argument. The return value is never used here, so I am removing it to simplify.

xymostech · 2017-10-11T23:05:18Z

    }

-    set(key /* : string */, value /* : any */) {
+    set(key /* : string */, value /* : any */, preserveOrder /* : boolean */) {


Hmm, could we maybe have preserveOrder default to true and set it to false in all of the places we explicitly want it to override? I think I'd assume that preserveOrder would be the default in something called OrderedElements...

We want this to be false in every place except one: string handlers. I'm happy to flip it to be reorder or shouldReorder and make sure that it is always passed in though if you think that is better.

Alright, I added a commit that inverts this!

xymostech · 2017-10-11T23:09:18Z

            this.keyOrder.push(key);
+        } else if (!preserveOrder) {
+            const index = this.keyOrder.indexOf(key);
+            this.keyOrder.push(this.keyOrder.splice(index, 1)[0]);


I think splitting this into two lines would be clearer

this.keyOrder.splice(index, 1); this.keyOrder.push(key);

Ah, good call.

oops, argh, I meant to put this into a review. Other comments incoming...

xymostech · 2017-10-11T23:10:01Z

+
+        elems.set("a", 1);
+        elems.set("b", 1);
+        elems.set("a", 2, { preserveOrder: true });


this isn't how the API works, right? It should just be a true here

Oh, right. This was an earlier revision. I'll fix now.

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.

@xymostech

As @xymostech pointed out in code review > I'd assume that preserveOrder would be the default in something called > OrderedElements This makes sense to me, so I am inverting this boolean and explicitly passing it in everywhere it is used now.

xymostech · 2017-10-11T23:11:53Z

    }

-    set(key /* : string */, value /* : any */) {
+    set(key /* : string */, value /* : any */, preserveOrder /* : ?boolean */) {


Could we make preserveOrder default to true and then explicitly set it to false in the places where we're overriding? I think it makes more sense for something called OrderedElements to preserve order by default.

xymostech · 2017-10-11T23:12:28Z

    stringHandlers /* : StringHandlers */,
    selectorHandlers /* : SelectorHandler[] */
-) /* : OrderedElements */ => {
+) /* : void */ => {


xymostech · 2017-10-11T23:13:12Z

+            }
+        }`, defaultSelectorHandlers);
+    });
+


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

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

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. :)

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.

lencioni · 2017-10-11T23:50:00Z

Thanks for the quick review!

xymostech · 2017-10-11T23:50:43Z

You're welcome! Thanks for fixing bugs! ❤️

Soreine · 2017-12-29T18:24:18Z

@lencioni I have run into this behavior just now. Let me state a related question:

I am making a component <Custom /> that uses a component <Base /> like so:

class Base extends React.Component {
    props: {
        styles: SheetDefinitions
    };

    render() {
        return (
            <div
                className={css(
                    {
                        '@media x': {
                            A
                        },
                        '@media y': {
                            B
                        }
                    },
                    this.props.styles
                )}
            />
        );
    }
}

class Custom extends React.Component {
    render() {
        return (
            <Base
                styles={{
                    '@media x': {
                        C
                    }
                }}
            />
        );
    }
}

I would like the generated CSS to be in this order (not to merge the media queries):

@media x {
  A
}

@media y {
  B
}

@media x {
  C
}

Instead, I get:

@media y {
  B
}

@media x {
  A,
  C
}

How does one avoid the merging of similar media queries ?

Return nothing from runStringHandlers

59084de

This method works by mutating its first argument. The return value is never used here, so I am removing it to simplify.

lencioni added the bug label Oct 11, 2017

lencioni requested a review from xymostech October 11, 2017 22:58

xymostech reviewed Oct 11, 2017

View reviewed changes

lencioni force-pushed the media-queries branch from dfce582 to 2dbfe26 Compare October 11, 2017 23:05

xymostech reviewed Oct 11, 2017

View reviewed changes

lencioni force-pushed the media-queries branch from 2dbfe26 to 01a3c83 Compare October 11, 2017 23:10

lencioni force-pushed the media-queries branch from 01a3c83 to 898c228 Compare October 11, 2017 23:10

xymostech approved these changes Oct 11, 2017

View reviewed changes

lencioni merged commit 1ca597d into master Oct 11, 2017

lencioni deleted the media-queries branch October 11, 2017 23:49

Conversation

lencioni commented Oct 11, 2017

Uh oh!

xymostech Oct 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lencioni commented Oct 11, 2017

Uh oh!

xymostech commented Oct 11, 2017

Uh oh!

Soreine commented Dec 29, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

xymostech Oct 11, 2017 •

edited

Loading