Skip to content

Conversation

@brentvatne
Copy link
Collaborator

If someone could read this over and give me some feedback it would be much appreciated.

Addresses #1390

Copy link
Contributor

Choose a reason for hiding this comment

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

setNativeProps is best used for continuous animations, to avoid the overhead of rendering the component hierarchy and reconciling many views. I believe we want to discourage it from an API perspective since it is imperative and stores state in the native layer (DOM, UIView, etc.)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

👍 should have mentioned the continuous animations use case

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

also should be sure to emphasize more that this shouldn't be something that you use frequently outside of that use case

@ccheever
Copy link
Contributor

Random nitpick:
might change "(avoid a render pass, useful with continuous animations)"
to "(to avoid a render pass, which is useful with continuous animations)" or something like that.

At a higher level, I would change the framing of "Only use this if you really need to" to more of a "When to use this and when not to and why not". I generally agree with @ide that performance is the main reason to use it, and that you are trading off complexity of implementation for small -- but sometimes critically important -- gains in UI smoothness, etc.

Overall, I think this is awesome! I think that this will go along way to making bits of React Native less mysterious and get more people building good stuff on it.

@brentvatne
Copy link
Collaborator Author

Great point @ccheever - updated accordingly

@brentvatne
Copy link
Collaborator Author

I'm happy with this now, thanks so much for the help @ide @ccheever ❤️

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants