Skip to content

Conversation

@webOS101
Copy link
Contributor

This allows for passing props into the editor from the wrapping app.

@kitten
Copy link
Contributor

kitten commented May 20, 2017

I'm not sure, I want to allow passing arbitrary props through the element through for now. And if so, it'll need to correctly append classnames, and proxy events in the callbacks :/

@webOS101
Copy link
Contributor Author

I can add proxy event handling and classname handling, if desired.

@webOS101
Copy link
Contributor Author

If you prefer a more surgical approach, for my use case I need blur/focus event handlers passed through.

@webOS101
Copy link
Contributor Author

className was already being merged correctly in the editor. I added code to forward the events that were being handled internally by the editor. I did not add a handler for the onChange event in LiveEditor, but could add one if desired. Lastly, I made sure to delete any children passed in props, it didn't seem like that would be a good idea to pass through. Interestingly(?), contentEditable is a prop on the editor but there was no way to control it before.

Copy link
Contributor

@kitten kitten left a comment

Choose a reason for hiding this comment

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

Ok, last request. Otherwise lgtm :)

const { contentEditable, className, style } = this.props
const { contentEditable, className, style, ...rest } = this.props
const { html } = this.state
delete rest.children;
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a rather non obvious v8 perf hog :) can you just manually set code to undefined for presentations below? Also you don't need to do anything about children

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting. I don't think the lifetime of rest is such that it will be affected by the V8 issue, but I shall make the change requested and leave children alone.

Copy link
Contributor Author

@webOS101 webOS101 May 20, 2017

Choose a reason for hiding this comment

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

Unfortunately, setting it to undefined does not have the desired effect. It's still a property on rest and causes a React warning: Unknown prop code on <pre> tag. I removed the delete on children and the unneeded semicolon. The only other option is destructuring out code and ignoring it, but I don't like unused parameter warnings much.

@webOS101
Copy link
Contributor Author

If you're interested, here's the React discussion on undefined vs delete and that warning: facebook/react#7266

@kitten
Copy link
Contributor

kitten commented May 20, 2017

@webOS101 Interesting. Seems I forgot about this behaviour due to the future shift in React towards not filtering anymore :) Let's get this merged then

@kitten kitten merged commit b95ace3 into FormidableLabs:master May 20, 2017
@webOS101
Copy link
Contributor Author

FWIW, I did a benchmark test of rendering 500 components using destructuring and again using delete. The difference, on average, was less than 1ms per run, with the destructuring being somewhat faster.

@webOS101 webOS101 deleted the editor-rest-props branch May 20, 2017 22:41
@webOS101
Copy link
Contributor Author

whoops, I labeled my tests backwards. delete was faster.

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.

2 participants