Add support for descendant selectors. #61

xymostech · 2016-04-20T23:34:04Z

Summary: This adds support for doing descendant style selection; that is,
changing child component styles based on the state/existence of a parent. This
selection in particularly useful when combined with the use of pseudo-classes,
by generating styles that look like .parent:hover .child, so the child styles
can depend on the active state of the parent.

Fixes #10

An example of the syntax that is added in this commit (and an example for the
explanation in this commit message):

const styles = StyleSheet.create({
    parent: {
        '>>child': {
            color: "red"
        },
        ':hover': {
            '>>child': {
                color: "blue"
            },
            '>>otherchild': {
                color: "white"
            }
        },
    },

    altparent: {
        ':hover': {
            '>>child': {
                color: "green"
            }
        }
    }
});

It turns out that the API for aphrodite doesn't play well with descendent
styles. In particular, it's difficult to let descendants have unique
classnames for themselves, at the same time as allowing merging between
styles which contain descendant selectors.

This pull request attempts to do both of these things. The code is a bit messy,
so I'll lay out what's going on in this pull request. Please ask questions, if
something is confusing! The Aphrodite code is nice because it's all fairly
easily understood, and I'm afraid that this adding too much complexity.

The basic flow of this diff is:

In StyleSheet.create, we recurse through the passed-in styles, and find
each of the descendant selectors (which have keys that look like >>blah). In
the example above, it would find parent['>>child'],
parent[':hover']['>>child'], parent[':hover']['>>otherchild'], and
altparent[':hover']['>>child']. In each place, we:
- generate a class name for that descendant selector. This is based on the
  class name of the parent class, as well as the key name.
  
  For example, if the class name for styles.parent was parent_abcdef, we
  might generate the class name parent_abcdef__child for parent['>>child'].
- tag the style by adding a _names object, with the class name as a key of
  the object.
  
  For example, parent['>>child'] would end up looking like { color: "red", _names: { parent_abcdef__child: true } }.
- collect a map of each of the keys (without the >> bit) to their class
  names.
  
  For example, for styles.parent, we would generate a map that looks like
  { child: "parent_abcdef__child", otherchild: "parent_abcdef__otherchild" }.
  We merge in the map from key to class name into the generated style, so that
  the class names can be accessed using a syntax like styles.parent.child.
When parent styles are passed into css(), their styles are merged
together. If one style overrides another's descendant styling, the _names
object will be merged together and will contain all of the associated class
names.
For example, when evaluating css(styles.parent, styles.altparent), we would
end up with merged styles looking like:
```
{
    '>>child': {
        color: "red",
        _names: { parent_abcdef__child: true },
    },
    ':hover': {
        '>>child': {
            color: "green",
            _names: {
                parent_abcdef__child: true,
                altparent_123456__child: true,
            },
        },
        '>>otherchild': {
            color: "white",
            _names: { parent_abcdef__otherchild: true },
        }
    }
}
```
We then generate a map from the descendent keys to all of the class names that
could be associated with a given key by recursing and looking at each of the
_names objects. For example, the map would look like:
```
{
    '>>child': ["parent_abcdef__child", "altparent_123456__child"],
    '>>otherchild': ["parent_abcdef__otherchild"]
}
```
When generating the styles, we look at this map and then generate styles for
each of the classnames listed. This is so that these styles will match up with
uses of both css(styles.parent.child) and css(styles.altparent.child).
For example, when generating the style[':hover']['>>child'] styles, we
generate:
```
.parent_abcdef-o_O-altparent_123456:hover .parent_abcdef__child { ... }
.parent_abcdef-o_O-altparent_123456:hover .altparent_123456__child { ... }
```
When descendant styles are passed into css(), like
css(styles.parent.child), we simply return the associated class name (in this
case, "parent_abcdef__child") in the output.

Test Plan:

npm run test
cd examples && npm run examples, then visit http://localhost:4114/ and see
that the last line starts green and when "Hover over me" is hovered, the
other part turns blue.

@zgotsch @jlfwong @kentcdodds @montemishkin

kentcdodds · 2016-04-21T01:04:18Z

README.md

+ }
+});
+```
+


Could we add the example css output of this?

kentcdodds · 2016-04-21T01:18:37Z

Tried to review where I could. I'm afraid I still have a very limited understanding of how all this stuff works, but most stuff looks good to me :D

jlfwong · 2016-04-21T13:17:14Z

README.md

+
+```js
+const styles = StyleSheet.create({
+ parent: {


Both in the original spec you described and in this documentation, it wasn't clear to me whether parent was just a style name or whether parent carries some special meaning. It seems to be the former, but is worth clarifying and perhaps using a different name. perhaps someContainer or something would be clearer?

jlfwong · 2016-04-21T13:49:31Z

Thanks for giving this so much thought in order to implement missing functionality while not compromising on the philosophy of Aphrodite, and thanks to @kentcdodds for the quick review!

I'm probably going to need to take multiple passes at this, but I'll start with the helpfully thorough PR description.

Can you start out the PR description with what this is trying to accomplish, not with what makes it difficult? If I understand correctly, this is specifically to address the fact that children of pseudo selectors are not implementable without hacks in Aphrodite, right? Even starting off by saying it fixes #10 would've been helpful (I see that it's indeed present at the bottom of the description).

.parent_abcdef-o_O-altparent_123456:hover .parent_abcdef__child { ... }
.parent_abcdef-o_O-altparent_123456:hover .altparent_123456__child { ... }

The ... in this case will be identical, right? If I'm understanding correctly, the hashes would end up being the same (the abcdef vs 123456 has me a little confused). If it's true that the hashes match, it might be better to jut throw out the name wholesale and avoid the need to generate duplicate styles, especially since we were tinkering with the idea of doing this on all styles to reduce bytes sent over the wire.

jlfwong · 2016-04-21T13:56:27Z

src/generate.js

+ classNamesForDescendant);
+}
+
+const generateCSSInner = (selector, style, stringHandlers,


This is getting pretty complicated, and could also be greatly aided by a JSdoc, IMO.

montemishkin · 2016-04-23T19:12:57Z

Thanks for giving this so much thought in order to implement missing functionality while not compromising on the philosophy of Aphrodite, and thanks to @kentcdodds for the quick review!

I'm probably going to need to take multiple passes at this...

Agreed!

Thoughts from my first pass:

Given your approach, and taking into account the helpful notes from @jlfwong and @kentcdodds, the implementation looks good!

I'll try to find some time ASAP to both take a deeper dive into the current implementation and also consider the overall design / approach.

natew · 2016-04-26T18:30:49Z

Just want to say this is a really great idea, especially for the :hover>child which is a really common pattern I've run into.

Why not just use > rather than >>?

kentcdodds · 2016-04-26T18:32:29Z

Why not just use > rather than >>?

There was discussion on that in the issue. Because > has meaning in CSS (direct child) and this would not follow that meaning (any descendant) we had to come up with something else.

xymostech · 2016-05-10T01:37:47Z

Okay, I got around to cleaning up this pull request a bit. Not much changed, but I added a bunch more docs so hopefully that should clear up some confusion.

@jlfwong, some responses to your comments:

Can you start out the PR description with what this is trying to accomplish, not with what makes it difficult? If I understand correctly, this is specifically to address the fact that children of pseudo selectors are not implementable without hacks in Aphrodite, right? Even starting off by saying it fixes #10 would've been helpful (I see that it's indeed present at the bottom of the description).

I added a better intro to the description, hopefully that should be more helpful about what's going on. Sorry, I was approaching this with all of the context in #10!

.parent_abcdef-o_O-altparent_123456:hover .parent_abcdef__child { ... }
.parent_abcdef-o_O-altparent_123456:hover .altparent_123456__child { ... }
The ... in this case will be identical, right? If I'm understanding correctly, the hashes would end up being the same (the abcdef vs 123456 has me a little confused). If it's true that the hashes match, it might be better to jut throw out the name wholesale and avoid the need to generate duplicate styles, especially since we were tinkering with the idea of doing this on all styles to reduce bytes sent over the wire.

You are correct, the ... is identical. Actually now that I think about it, maybe I should be combining those with a comma (somehow forgot that CSS supports that!) The hashes would not be the same here, since they're just using the hashes we get from hashing the parent and altparent objects.

@zgotsch

Summary: It turns out descendant selectors are kinda hard. In particular, it's difficult to let *descendants have unique classnames* for themselves, at the same time as allowing *merging between styles which contain descendant selectors*. This pull request attempts to do both of these things. The code is a bit messy, so I'll lay out what's going on in this pull request. Please ask questions, if something is confusing! The Aphrodite code is nice because it's all fairly easily understood, and I'm afraid that this adding too much complexity. An example of the syntax (and an example for the explanation in this commit message), consider: ```js const styles = StyleSheet.create({ parent: { '>>child': { color: "red" }, ':hover': { '>>child': { color: "blue" }, '>>otherchild': { color: "white" } }, }, altparent: { ':hover': { '>>child': { color: "green" } } } }); The basic flow of this diff is: 1. In `StyleSheet.create`, we recurse through the passed-in styles, and find each of the descendant selectors (which have keys that look like `>>blah`). In the example above, it would find `parent['>>child']`, `parent[':hover']['>>child']`, `parent[':hover']['>>otherchild']`, and `altparent[':hover']['>>child']`. In each place, we: - generate a class name for that descendant selector. This is based on the class name of the parent class, as well as the key name. For example, if the class name for `styles.parent` was `parent_abcdef`, we might generate the class name `parent_abcdef__child` for `parent['>>child']`. - tag the style by adding a `_names` object, with the class name as a key of the object. For example, `parent['>>child']` would end up looking like `{ color: "red", _names: { parent_abcdef__child: true } }`. - collect a map of each of the keys (without the `>>` bit) to their class names. For example, for `styles.parent`, we would generate a map that looks like `{ child: "parent_abcdef__child", otherchild: "parent_abcdef__otherchild" }`. We merge in the map from key to class name into the generated style, so that the class names can be accessed using a syntax like `styles.parent.child`. 2. When *parent* styles are passed into `css()`, their styles are merged together. If one style overrides another's descendant styling, the `_names` object will be merged together and will contain all of the associated class names. For example, when evaluating `css(styles.parent, styles.altparent)`, we would end up with merged styles looking like: ``` { '>>child': { color: "red", _names: { parent_abcdef__child: true }, }, ':hover': { '>>child': { color: "green", _names: { parent_abcdef__child: true, altparent_123456__child: true, }, }, '>>otherchild': { color: "white", _names: { parent_abcdef__otherchild: true }, } } } ``` We then generate a map from the descendent keys to all of the class names that could be associated with a given key by recursing and looking at each of the `_names` objects. For example, the map would look like: ``` { '>>child': ["parent_abcdef__child", "altparent_123456__child"], '>>otherchild': ["parent_abcdef__otherchild"] } ``` When generating the styles, we look at this map and then generate styles for each of the classnames listed. This is so that these styles will match up with uses of both `css(styles.parent.child)` and `css(styles.altparent.child)`. For example, when generating the `style[':hover']['>>child']` styles, we generate: ``` .parent_abcdef-o_O-altparent_123456:hover .parent_abcdef__child { ... } .parent_abcdef-o_O-altparent_123456:hover .altparent_123456__child { ... } ``` 3. When *descendant* styles are passed into `css()`, like `css(styles.parent.child)`, we simply return the associated class name (in this case, `"parent_abcdef__child"`) in the output. Fixes #10 Test Plan: - `npm run test` - `cd examples && npm run examples`, then visit http://localhost:4114/ and see that the last line starts green and when "Hover over me" is hovered, the other part turns blue. @zgotsch @jlfwong @kentcdodds @montemishkin

xymostech · 2016-05-10T19:17:39Z

Okay, I pushed a change that uses commas to join things instead of duplicating styles, hopefully that's a little better.

I also ran a code coverage tool and found a case I hadn't tested. I'm gonna go back and add tests for the other things we aren't covering right now, maybe we could add a test that ensures aphrodite stays at 100% code coverage!

kentcdodds · 2016-05-10T19:27:40Z

maybe we could add a test that ensures aphrodite stays at 100% code coverage!

You may consider adding a check-coverage script and add that as a git hook using ghooks (like this, though these day's I recommend nyc). I'd recommend also using opt-cli so new contributors can get involved without being blocked by coverage.

Just some thoughts :-)

jlfwong · 2016-05-12T15:49:47Z

src/generate.js

 } from './util';

+/**


Awww yeaaahhh JSdoc 💖

kentcdodds · 2016-05-12T16:04:52Z

FYI, this PR can no longer merge as it has conflicts.

jlfwong · 2016-05-12T16:06:32Z

src/generate.js

+ *
+ * Example:
+ * ```
+ * generateCSSRuleset(".blah", { color: "red" });


Would be good to include an example that involves a pseudo selector so show that selector isn't necessarily just a classname selector

jlfwong · 2016-05-12T16:16:33Z

src/index.js

@@ -49,11 +80,23 @@ const css = (...styleDefinitions) => {
 return "";
 }

- const className = validDefinitions.map(s => s._name).join("-o_O-");
+ // Filter out "plain class name" arguments, which just want us to add a
+ // classname to the end result, instead of generating styles.


I think it's probably helpful to say "this is the case, e.g., for descendent selectors where the relevant CSS is generated by the call to css(styles.foo), not the call to `css(styles.foo.someChild)"

jlfwong · 2016-05-12T17:19:43Z

Okay, after a second pass, I think I have a better understanding of what this is doing, but am still fuzzy on a few details, and am becoming a little uneasy about some behaviour (if I understand correctly how this works) that would be surprising to me.

Here are my major reservations about the API design:

If you use a child selector without using the associated parent selector, nothing happens.

If I'm understanding the code correctly, if you have the following:

const styles = StyleSheet.create({
    foo: {
        '>>someChild': { color: 'red'; }
    }
});

And in your component, you have:

<div>
     <div className={css(styles.foo.someChild)}>Yo</div>
</div>

Then the div containing Yo will receive no styles, but WILL receive a class name. This is especially bad when combined with a silent failure from a typo, like so:

<div className={css(styles.fo)}>
     <div className={css(styles.foo.someChild)}>Yo</div>
</div>

Because of multiple names, there are multiple ways of referencing the same child.

Having multiple ways of accomplishing the same thing generally acts as a red flag of API design to me, but perhaps it's not sensibly avoidable in this case...?

const styles = StyleSheet.create({
    foo: {
        '>>someChild': { color: 'red'; }
    },
    bar: {
        '>>someChild': { color: 'green'; }
    }
});

Then, if I'm understanding correctly, the following are equivalent from a user perspective and will generate the same CSS (though not HTML?):

<div className={css(styles.foo, styles.bar)}>
     <div className={css(styles.foo.someChild)}>
</div>

and

<div className={css(styles.foo, styles.bar)}>
     <div className={css(styles.bar.someChild)}>
</div>

and

<div className={css(styles.foo, styles.bar)}>
     <div className={css(styles.bar.someChild, styles.foo.someChild)}>
</div>

The semantics of nested descendent selectors is confusing to me

...or possibly not implemented correctly? The code goes through all the trouble of identifying what the possibly parent selectors might be, but from the looks of it, it wasn't done recursively? If you have this:

const styles = StyleSheet.create({
    a1: {
        '>>b': { '>>c': { color: 'red'; } }
    },
    a2: {
        '>>b': { '>>c': { color: 'green'; } }
    }
});

Then what happens if you do this?

<div className={css(a1, a2)}>
     <div className={css(a1.b)}>
        <div className={css(a2.c})>
        </div>
    </div>
</div>

Overall, the semantics of any examples involving multiple instances of >>someChild aren't terribly clear to me (both in mergers and in pseudo selectors). Are there any valid use cases of allowing nested descendent selectors that aren't accomplishable in some other way? How much simpler does the design become if we restrict descendant selectors to only being immediate children of pseudo-selectors, and not allowing nesting of descendant selectors?

It's also not intuitive to me that >>b is addressed as a1.b, but >>c is not addressed as a1.b.c, though I guess it's okay because a2 is not the same kind of thing as b?

Do others find the semantics from a user-standpoint confusing here? I'm particularly interested in @zgotsch thoughts, who may be able to draw from his experience in a real codebase that resulted in #10 being opened in the first place.

From a code complexity standpoint, this change seriously impedes my ability to reason intuitively about what the behavior of the code will be in all cases, but I didn't see any unnecessary complexity given this API design, so it may be necessary evil.

xymostech · 2016-05-12T21:09:35Z

@jlfwong Thank you for the review of this API! I agree, it's kinda confusing. I'm also super sad that this change makes the code more difficult to understand, so I'd love to figure out an API that resulted in more intuitive code. I couldn't come up with a better way to do it myself, but maybe together we can brainstorm something? Gonna respond to your different points:

If you use a child selector without using the associated parent selector, nothing happens.

That is correct. I'm not super worried about this one though, because it's the same as how normal CSS works, no? It does make the "these descendant selectors are special" more apparent though.

Because of multiple names, there are multiple ways of referencing the same child.

Yeah, this is fugly. You are correct, those are all style-equivalent. I guess I chose this because it seems like it would lead to the least amount of things-not-working. If you have some parent selectors and some associated child selectors, then any combination should Just Work. It seemed simpler from an external API perspective, but leads to complex code.

The semantics of nested descendent selectors is confusing to me

They're kinda confusing to me too. It wouldn't be too much work to make a1.b.c work, but it's not clear that that's any better. We could just disallow nested child selectors, but that might be more confusing? I guess if we threw an error that would be okay.

wrt. your example, I have no idea what happens there. Actually maybe that doesn't work any more now that I implemented the comma stuff. Let's go check!

I guess overall, I really like the API for simple cases but gets much more confusing with complex cases. Maybe something similar but simplified would be great? I'm not at all attached to this implementation, it just ended up having a really clean external API (as opposed to the cssWithDescendants stuff, which ended up being really hard to use).

I'm reluctant to let #61 land because of the complexity it introduces, but would like these docs to land (slightly modified from #61), and would prefer to not make them landing dependant on the complexity of #61.

iamrane · 2016-08-19T10:42:35Z

Why not using the more sass syntax

'& .child': {
   padding: 10
}

xymostech · 2016-08-22T19:29:57Z

@iamrane The reasons we're not landing this is because the API is too complex/confusing. Once that gets worked out, we could figure out what syntax we would like to use.

aesopwolf · 2016-09-03T00:02:14Z

Any thoughts on using just a space as the child selector instead of two angle brackets >>child?

' child': {
    color: "red"
}

marcusradell · 2016-09-05T10:02:43Z

@aesopwolf I feel white spaces are dangerous due to the low visibility. Try find a bug where you missed a white space or had the wrong number or wrong kind. It's not fun. Same issue with O/0 and other ambiguous characters.

xymostech · 2016-10-11T00:36:52Z

Okay, I'm going to close this PR since it's stagnated a bunch and I'm not super enthusiastic that this approach is going to work. All discussion should continue in #10.

The comments for `generateCSS` included descendant selector example from closed PR Khan#61

The comments for `generateCSS` included descendant selector example from closed PR #61

kentcdodds reviewed Apr 21, 2016
View reviewed changes

jlfwong reviewed Apr 21, 2016
View reviewed changes

viniciusdacal mentioned this pull request May 9, 2016

Support pseudo states with nested children #72

Closed

xymostech force-pushed the descendent-styles branch 2 times, most recently from bcf550f to 564fa7f Compare May 10, 2016 01:28

xymostech force-pushed the descendent-styles branch from 564fa7f to 56de8c4 Compare May 10, 2016 19:16

jlfwong reviewed May 12, 2016
View reviewed changes

src/generate.js

} from './util';

/**

Copy link

Collaborator

jlfwong May 12, 2016

Choose a reason for hiding this comment

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

Awww yeaaahhh JSdoc 💖

jlfwong reviewed May 12, 2016
View reviewed changes

jlfwong mentioned this pull request May 16, 2016

How to handle descendant selection of pseudo-states? #10

Closed

jlfwong mentioned this pull request Jun 7, 2016

Document generateCSS and generateCSSRuleset methods #87

Merged

kentcdodds mentioned this pull request Jun 7, 2016

How do you style child element with Aphrodite based on parent's pseudo state? kentcdodds/ama#136

Closed

jlfwong mentioned this pull request Jun 30, 2016

Allow additional handlers for special selectors to be added. #95

Merged

xymostech closed this Oct 11, 2016

borkxs added a commit to borkxs/aphrodite that referenced this pull request Jan 23, 2017

Remove mentions of unimplemented feature

5b3ebd2

The comments for `generateCSS` included descendant selector example from closed PR Khan#61

borkxs mentioned this pull request Jan 23, 2017

Remove mentions of unimplemented feature #186

Merged

xymostech pushed a commit that referenced this pull request Jan 23, 2017

Remove mentions of unimplemented feature (#186)

889d7f0

The comments for `generateCSS` included descendant selector example from closed PR #61

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for descendant selectors. #61

Add support for descendant selectors. #61

xymostech commented Apr 20, 2016 •

edited

Loading

kentcdodds Apr 21, 2016

xymostech May 10, 2016

kentcdodds commented Apr 21, 2016

jlfwong Apr 21, 2016 •

edited

Loading

xymostech May 10, 2016

jlfwong commented Apr 21, 2016

jlfwong Apr 21, 2016

montemishkin commented Apr 23, 2016

natew commented Apr 26, 2016

kentcdodds commented Apr 26, 2016

xymostech commented May 10, 2016

xymostech commented May 10, 2016

kentcdodds commented May 10, 2016

jlfwong May 12, 2016

kentcdodds commented May 12, 2016

jlfwong May 12, 2016

jlfwong May 12, 2016 •

edited

Loading

jlfwong commented May 12, 2016

xymostech commented May 12, 2016

iamrane commented Aug 19, 2016 •

edited

Loading

xymostech commented Aug 22, 2016

aesopwolf commented Sep 3, 2016 •

edited

Loading

marcusradell commented Sep 5, 2016

xymostech commented Oct 11, 2016

Add support for descendant selectors. #61

Add support for descendant selectors. #61

Conversation

xymostech commented Apr 20, 2016 • edited Loading

kentcdodds Apr 21, 2016

Choose a reason for hiding this comment

xymostech May 10, 2016

Choose a reason for hiding this comment

kentcdodds commented Apr 21, 2016

jlfwong Apr 21, 2016 • edited Loading

Choose a reason for hiding this comment

xymostech May 10, 2016

Choose a reason for hiding this comment

jlfwong commented Apr 21, 2016

jlfwong Apr 21, 2016

Choose a reason for hiding this comment

montemishkin commented Apr 23, 2016

natew commented Apr 26, 2016

kentcdodds commented Apr 26, 2016

xymostech commented May 10, 2016

xymostech commented May 10, 2016

kentcdodds commented May 10, 2016

jlfwong May 12, 2016

Choose a reason for hiding this comment

kentcdodds commented May 12, 2016

jlfwong May 12, 2016

Choose a reason for hiding this comment

jlfwong May 12, 2016 • edited Loading

Choose a reason for hiding this comment

jlfwong commented May 12, 2016

If you use a child selector without using the associated parent selector, nothing happens.

Because of multiple names, there are multiple ways of referencing the same child.

The semantics of nested descendent selectors is confusing to me

xymostech commented May 12, 2016

iamrane commented Aug 19, 2016 • edited Loading

xymostech commented Aug 22, 2016

aesopwolf commented Sep 3, 2016 • edited Loading

marcusradell commented Sep 5, 2016

xymostech commented Oct 11, 2016

xymostech commented Apr 20, 2016 •

edited

Loading

jlfwong Apr 21, 2016 •

edited

Loading

jlfwong May 12, 2016 •

edited

Loading

iamrane commented Aug 19, 2016 •

edited

Loading

aesopwolf commented Sep 3, 2016 •

edited

Loading