RFC: Accept arrays of styles in css() #154
Conversation
|
@ide Thanks for the patch, with tests no less! @xymostech Thoughts? I'm happy with doing something like this, though I'd prefer it be recursive as well. |
|
I'm pretty happy about merging this! I think that the marginal increase in API complexity is worth it, especially since this is what React Native does. I agree though that this should almost certainly be recursive, otherwise people will be confused about when nesting lists is acceptable. |
|
@jlfwong @xymostech thanks! I'll make the flattening be recursive -- are you ok with an extra dependency (https://www.npmjs.com/package/lodash.flattendeep) or would you prefer flattenDeep to live under utils.js? |
|
I'd be fine with the dependency, but I'm never sure how small the individual lodash packages are. Can you add the dependency and see how big the built file gets? |
|
I'd prefer no dependency. We don't need to cover as many cases as the lodash package does. It seems pretty expansive: https://github.com/lodash/lodash/blob/4.4.0-npm-packages/lodash.flattendeep/index.js. It deals with things like Does React Native support non-Array types like iterators or enumerable things? |
|
React Native supports only arrays I think. Supporting other iterables like Sets could be nice though, iterall makes that easy and is fairly lightweight, it's 600KB minified and gzipped. https://github.com/leebyron/iterall Do you prefer supporting iterables or just arrays? |
|
Here's the relevant React Native source (I think). https://github.com/facebook/react-native/blob/master/Libraries/StyleSheet/flattenStyle.js Arrays only, please! |
|
You'll also need to sign this in case you haven't already https://docs.google.com/a/figma.com/forms/d/e/1FAIpQLSdyXYrc8ogVoA46J9KXyIj5nKlZzNkOnQG-4A1R7X_BWGTShQ/viewform |
|
I signed the CLA and added |
|
Awesome. I'm happy with this if you are, @jlfwong! |
|
Hooray! Last thing is to add a section to the README documenting this, and then it's good to go! I think it should go right below "Conditionally Applying Styles" |
Accepting both styles and arrays of styles improves developer ergonomics with how I use Aphrodite when passing styles into components. The biggest problem I run into is that the spread operator doesn't work with undefined, so this code fails:
```js
// Component.js
class Component extends React.Component {
static propTypes = { styles: PropTypes.array };
render() {
// this css() call fails because spreading `undefined` is an error
return <div className={css(styles.myStyle, ...this.props.styles)} />;
}
}
const styles = StyleSheet.create({...});
// OwnerComponent.js
class OwnerComponent extends React.Component {
render() {
return <Component />; // we're happy with the default styles
}
}
```
The second, more cosmetic issue, is that even if I'm passing down just one style I need to create an array:
```js
// OwnerComponent.js
class OwnerComponent extends React.Component {
render() {
return <Component styles={[styles.customStyle]} />; // need the [ ]
}
}
const styles = StyleSheet.create({...});
```
With this PR, you'd write `css(styles.myStyle, this.props.styles)` and it'd address both these issues. Aphrodite would flatten `this.props.styles` if it were an array, it would leave it alone if it were an Aphrodite style object, and it would also leave it alone and filter it out if it were falsy.
As a data point, React Native flattens style arrays (recursively, actually) and it's pretty convenient with little downside. So just wanted to float this idea out there.
Test Plan: added unit tests
|
Added a README section (preview here https://github.com/ide/aphrodite/tree/css-arrays#combining-styles). I used the example of combining styles from props since that answers some questions that other people have asked in a couple of older issues. |
|
@ide thanks for signing our CLA. |
|
Sweeet - let's get this landed! |
Accepting both styles and arrays of styles improves developer ergonomics with how I use Aphrodite when passing styles into components. The biggest problem I run into is that the spread operator doesn't work with undefined, so this code fails:
The second, more cosmetic issue, is that even if I'm passing down just one style I need to create an array:
With this PR, you'd write
css(styles.myStyle, this.props.styles)and it'd address both these issues. Aphrodite would flattenthis.props.stylesif it were an array, it would leave it alone if it were an Aphrodite style object, and it would also leave it alone and filter it out if it were falsy.As a data point, React Native flattens style arrays (recursively, actually) and it's pretty convenient with little downside. So just wanted to float this idea out there.
Test Plan: added unit tests