-
Notifications
You must be signed in to change notification settings - Fork 67
Initial Image and/or Custom Image Data Prop #33
base: master
Are you sure you want to change the base?
Conversation
Thanks for your hard work @hnryjms ! I'll have a play with it over the weekend. I'm slightly concerned about the double rendering issue though... What was your other idea around it? |
What I was thinking is that as long as JS code keeps pushing updates back to ObjC, we wouldn't need to render at The only thing I'm unsure on without testing this approach is if |
@@ -33,11 +35,17 @@ export default class Sketch extends React.Component { | |||
flex: 1, | |||
backgroundColor: 'transparent', | |||
}; | |||
|
|||
this.state = { | |||
imageData: props.imageData |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found an issue where if the prop is undefined
here, removing one <Sketch
from the DOM and then rendering a new one later will cause the sketch to contain the old drawing. Adding || null
to this initial state causes the issue to go away--RCT will pass the null value inward as nil
, but won't pass undefined
as a nil
to ObjC.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- done by fixing
defaultProps
Hey @hnryjms! I'm currently having a look at your PR. Can you give me an example of how I should be using |
So it takes in RNFS.readFile(initialSketchImageFile.path, 'base64')
.then((base64) => {
const imageData = 'data:image/jpg;base64,' + base64;
this.setState({ imageData });
}) |
^ I'm not in love with that whole Maybe we want a helper function like the one below? thoughts?? RNSketch.getImageDataFromFilePath(initialSketchImageFile.path)
.then((imageData) => {
this.setState({ imageData });
}); |
Hey @jgrancher I just updated this to add a helper function. I'm not in love with making this Sketch project force Maybe give this a shot, and we can go from there. I'll also try to spin back up here and build that |
This is a rebuild of my coworker @gensc004's work to allow initial images to the Sketchpad. I have more past experience with ObjC code so I worked on upgrading it to work inline with the change that happened in this library since #27 was opened.
The implementation I built here is a new prop,
imageData
that is passed into the native component. Here's what happens:<Sketch
is passedimageData
as a prop at first render.state
of the JS component, and sent to the native component.onChange()
when a line is drawn.state
and reappliesimageData
to the native component <- this causes double rendering of the line...once immediately after the line is drawn, and again once the JS code "accepts" the line.imageData
value passed to<Sketch
will overwrite the current value rendered in native code. If the parent component callssetState()
within theonChange()
function's callback, the change will be deduplicated by the React reconciler and only the parent component's value will be applied.For me, the double rendering resolves an issue where the JS state is out-of-sync with the ObjC state for this component. Ideally, we would only need to render once...but the solution to this could be in a few different ways...I would vote to either leave the double rendering as there's minimal performance impact, or have the end-of-line-drawn not trigger the render, and rely on the JS state for accurate
imageData
information.Remaining Tasks