Skip to content

Conversation

ManasJayanth
Copy link
Contributor

Indicates the purpose and an alternative use for the method too.

@facebook-github-bot
Copy link
Contributor

By analyzing the blame information on this pull request, we identified @spicyj, @corbt and @vjeux to be potential reviewers.

@facebook-github-bot facebook-github-bot added GH Review: review-needed CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. labels Mar 25, 2016
@vjeux
Copy link
Contributor

vjeux commented Mar 25, 2016

Thanks! This is super useful.

Do you mind adding a big warning at the top that this is not a recommended method and is an escape hatch. Style objects being opaque is required if we want to do optimizations down the line or work on the web by outputting classes, if you use this method, you are going to break this assumption.

@facebook-github-bot
Copy link
Contributor

@prometheansacrifice updated the pull request.

@ManasJayanth
Copy link
Contributor Author

@vjeux Do you think the following is valid use case?

// Child
render() {
  const overridingViewStyles = StyleSheet.flatten(this.props.style).viewStyles; //Ignore the fact that I could have cached it somewhere else instead of the render method. Bear with it for brevity.
  return <View style={[styles.defaultStyles, overridingViewStyles]} />
}
//Parent
render() {
  return <Child style={styles.childStyles} />
}

Basically, individual components nested within the child component, obtain their styles from style property. This would be possible only if the child component had access to the style objects passed by the parent.

@vjeux
Copy link
Contributor

vjeux commented Mar 25, 2016

You can do more simply:

// Child
render() {
  return <View style={[styles.defaultStyles, this.props.style]} />
}

@vjeux
Copy link
Contributor

vjeux commented Mar 25, 2016

You don't need to flatten it at every step of the way, and really, you don't want to flatten it at every step of the way which is actually pretty expensive. You just want to forward it all the way down.

If we can even get it unflattened all the way to React, then we can build good dev tools where we can display how the styles were combined and overridden like we have on web.

@vjeux
Copy link
Contributor

vjeux commented Mar 25, 2016

@facebook-github-bot shipit

@facebook-github-bot
Copy link
Contributor

Thanks for importing. If you are an FB employee go to Phabricator to review.

@ManasJayanth
Copy link
Contributor Author

@vjeux The issue with style={[styles.defaultStyles, this.props.style]} was that sometimes you have styles that apply to different children components (and specifically to those alone)
Say for example a child is a simple Text nested inside a View. When I pass styles to Child like this,

<Child style={styles.childStyles} />

both View and Text obtain all the styles, which can cause warnings. There needs to be a way to separate out the style passed on to Child to its children Text and View

@sophiebits
Copy link
Contributor

It is traditional to pass them as separate props.

<Component
  style={....}
  childStyle={...} />

Then you can just use props.childStyle.

@vjeux
Copy link
Contributor

vjeux commented Mar 25, 2016

I tried to document this use case here: http://facebook.github.io/react-native/docs/style.html#content

@ManasJayanth
Copy link
Contributor Author

Thank you for your inputs @spicyj @vjeux

@ghost ghost closed this in 9b9cc6b Mar 25, 2016
@ManasJayanth
Copy link
Contributor Author

@vjeux @spicyj I noticed that style.length is not cached in the for loop (here). Replace it with for (var i = 0, ii = style.length; i < ii; ++i) {?

@satya164
Copy link
Contributor

@prometheansacrifice Why not l instead of ii?

@ManasJayanth
Copy link
Contributor Author

@satya164 Sure. But open a new PR for this?

@satya164
Copy link
Contributor

@prometheansacrifice Why not :)

@ManasJayanth
Copy link
Contributor Author

@satya164 Did so here :)

zebulgar pushed a commit to nightingale/react-native that referenced this pull request Jun 18, 2016
Summary:Indicates the purpose and an alternative use for the method too.
Closes facebook#6662

Differential Revision: D3099823

Pulled By: vjeux

fb-gh-sync-id: 44633161a3df9b11d44afaed72fe6127f0b6bf7b
fbshipit-source-id: 44633161a3df9b11d44afaed72fe6127f0b6bf7b
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants