Skip to content
This repository has been archived by the owner on Oct 8, 2021. It is now read-only.

Proposal: add setNativeProps to Group, Shape, Text #41

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

MorganIsBatman
Copy link

@MorganIsBatman MorganIsBatman commented Nov 16, 2019

Summary

This PR adds setNativeProps methods to the <Group/>, <Shape/>, and <Text/> components provided by art to allow direct manipulation of the native component, as per https://facebook.github.io/react-native/docs/direct-manipulation.

A benefit of this change is that we can use Animated.Values to animate props like opacity without the animation calling the render method each frame (currently forceUpdate is fired to update the component, causing a rerender).

My personal motivation is to use react-native-reanimated with art, which requires this change to work, but I believe it has value more generally.

Notes/discussion points:

  • I've created a method to translate props to native props, and used this both in the render and setNativeProps methods for each component. This may differ from what a user would expect, where setNativeProps suggests passing its arguments straight to the native component. I've done this because I think most users are going to want to set props in the render method, and then directly manipulate them in a consistent manner, relying on art to translate the props to what the native component requires. If we were to pass the arguments directly through, many of them would need to be a different format to their equivalent prop, which I think adds unnecessary complexity
  • for each component, setNativeProps merges existing props into the new props before translation. The method we use does provide for only translating the props that would change as a result of the new props passed in (we pass an array of the props required to be translated), but I suspect the logic to work out what those props would be would be as expensive to compute as the 'over computing' we are undertaking by translating all props, every time.

Test Plan

I've tested (on iOS simulator, Android emulator and real iPhone 8) that:

  • the components render correctly, as they did before the change (no detrimental impact)
  • using an Animated.Value for opacity, strokeWidth, stroke, and fill will correctly animate the <Shape/> component without firing the render method

What's required for testing (prerequisites)?

  • I guess we should probably test on a real android device?

What are the steps to reproduce (after prerequisites)?

  • N/A

Compatibility

OS Implemented
iOS
Android ✅ - emulator tested only

Checklist

  • [X ] I have tested this on a device and a simulator (not android device)
  • [ N/A] I added the documentation in README.md
  • [N/A ] I mentioned this change in CHANGELOG.md
  • [X ] I updated the typed files (TS and Flow)
  • [N/A ] I added a sample use of the API in the example project (example/App.js)

- new `translatePropsToNativeProps` helper to return required nativeProps translated from props
- change render method of `<Group/>`, `<Shape/>`, `<Text/>` to use the method above to translate props to nativeProps
- implement `setNativeProps` in the above three components, using the same helper to translate changed and existing props into nativeProps.
@Esemesek
Copy link
Collaborator

Sorry for the delay on this one. It seems like a valid use case. I'll make sure to check it out in a day or two. Can you also include some example Animations in the example app?

@MorganIsBatman
Copy link
Author

All good! Absolutely - I'll sort those out this weekend :)

A simple 3-flashing circle animation to demonstrate animation.

Note: this animation would still be possible without the addition of `setNativeProps`, however it would trigger a rerender of the Shape component for each step of the animation.
@MorganIsBatman MorganIsBatman force-pushed the feature/add-setNativeProps-to-components branch from c73f3a2 to c69c501 Compare December 22, 2019 23:04
@MorganIsBatman
Copy link
Author

@Esemesek, I've added a simple animation to the example app, animating the opacity of 3 circles (sorry for the lack of imagination on that one!), and updated the code to use the new shadow props provided in #45.
Art_Animated

@MorganIsBatman
Copy link
Author

@Esemesek - is there anything more you need me to do with this? I have a few days spare next week that I can work on it if there's anything that needs doing.

@Esemesek
Copy link
Collaborator

@MorganIsBatman It seems alright. I am quite hesitant because I don't want to break anything. The example looks really cool C: Will try to progress this as soon as I find more time.

@MorganIsBatman
Copy link
Author

Great. Thanks for your response @Esemesek :)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants