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

Initial Image and/or Custom Image Data Prop #33

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

Conversation

hnryjms
Copy link
Contributor

@hnryjms hnryjms commented Oct 5, 2017

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:

  1. <Sketch is passed imageData as a prop at first render.
  2. Prop is copied to state of the JS component, and sent to the native component.
  3. (later): Native component calls onChange() when a line is drawn.
  4. JS component updates state and reapplies imageData 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.
  5. (opt): At any time, a new imageData value passed to <Sketch will overwrite the current value rendered in native code. If the parent component calls setState() within the onChange() 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

  • Discuss & agree on double-rendering ObjC code
  • Add documentation for the new property

@jgrancher
Copy link
Owner

jgrancher commented Oct 6, 2017

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?

@hnryjms
Copy link
Contributor Author

hnryjms commented Oct 6, 2017

What I was thinking is that as long as JS code keeps pushing updates back to ObjC, we wouldn't need to render at -touchesEnded:withEvent:, because we can assume the JS will send the new imageData value once it's processed in the JS code of the app (triggering the rerender). -touchesEnded:withEvent: would instead act the same as -touchesMoved:withEvent: for placing the last point onto the line.

The only thing I'm unsure on without testing this approach is if -drawingToString will send the updated imageData value, or if it won't include the most recent line.

@@ -33,11 +35,17 @@ export default class Sketch extends React.Component {
flex: 1,
backgroundColor: 'transparent',
};

this.state = {
imageData: props.imageData
Copy link
Contributor Author

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

  • done by fixing defaultProps

@jgrancher
Copy link
Owner

Hey @hnryjms! I'm currently having a look at your PR. Can you give me an example of how I should be using imageData? Seems that I can't render any initial image right now.

@hnryjms
Copy link
Contributor Author

hnryjms commented Nov 14, 2017

So it takes in base64 encoded image data, in the same format that is returned as the onChange() function. Here's the code my team is using to pass an initial image in:

RNFS.readFile(initialSketchImageFile.path, 'base64')
    .then((base64) => {
        const imageData = 'data:image/jpg;base64,' + base64;
        this.setState({ imageData });
    })

@hnryjms
Copy link
Contributor Author

hnryjms commented Nov 14, 2017

^ I'm not in love with that whole base64 thing....a path would seem more practical. My only thought is that when it takes base64 data, it is consistent inputs and outputs for the component.

Maybe we want a helper function like the one below? thoughts??

RNSketch.getImageDataFromFilePath(initialSketchImageFile.path)
    .then((imageData) => {
        this.setState({ imageData });
    });

@hnryjms
Copy link
Contributor Author

hnryjms commented Jul 13, 2018

Hey @jgrancher I just updated this to add a helper function. I'm not in love with making this Sketch project force react-native-fs upon everyone, but I think it's clearer than forcing the caller of that helper function to pass in pre-loaded base64 encoded image data. Unsure about the thoughts on this....it might be better to write the one operation (RNFS.readFile( ... , 'base64')) into the Sketch ObjC code itself, then there's no new dependency.

Maybe give this a shot, and we can go from there. I'll also try to spin back up here and build that -[RNSkecth readImageDataFile:] ObjC function so we don't need RNFS if I see this PR is sitting too long a second time.

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.

3 participants