Skip to content

Conversation

@shybyte
Copy link
Contributor

@shybyte shybyte commented Aug 8, 2017

This PR fixes #95.

I'm not really sure why the props.src duplication in state and in ObjectAttributes is necessary, instead of just using the props.src directly. I'm also not sure if componentDidUpdate is the right place to update ObjectAttributes but at least it passes all tests. I have also added a test to ensure the fixed behavior.

… props.src instead of the current props.src
@mac-s-g
Copy link
Owner

mac-s-g commented Aug 8, 2017

this is awesome! thank you!

I really appreciate the PR! I'll try to review by the end of the day.

@mac-s-g
Copy link
Owner

mac-s-g commented Aug 8, 2017

I misunderstood your point about index.state and ObjectAttributes duplicating storage of src at first.

I'll take another look at that to see if I can consolidate to improve readability

@shybyte
Copy link
Contributor Author

shybyte commented Aug 8, 2017

I could imagine to just render props.src and get rid of this.state.src and ObjectAttributes completely. In this case the client which implements onEdit would be responsible to put updated_src back into props as src. These are superficial thoughts. Don't take it to seriously. I just have the feeling that this bug occurred because ".src" lives in 3 places (props, state, ObjectAttributes).

Have a look at https://facebook.github.io/react/docs/react-component.html:

Beware of this pattern, as state won't be up-to-date with any props update. Instead of syncing props to state, you often want to lift the state up. If you "fork" props by using them for state, you might also want to implement componentWillReceiveProps(nextProps) to keep the state up-to-date with them. But lifting state up is often easier and less bug-prone.

@mac-s-g
Copy link
Owner

mac-s-g commented Aug 9, 2017

yes, control could be completely moved to the client. My vision was to equip this component with a bit more functionality out of the box.

PR looks good though! - all tests are passing on my end.

I'm going to merge.

thanks again for posting this. Don't hesitate to post more PR's if you would like to see additional changes in behavior or code quality.

@mac-s-g mac-s-g merged commit 5ddf14f into mac-s-g:master Aug 9, 2017
This was referenced Aug 9, 2017
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.

The updated_src after an onEdit restores the initial props.src instead of the current props.src

2 participants