Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

A few ideas for tweaks #1

Closed
petehunt opened this issue Dec 16, 2013 · 5 comments
Closed

A few ideas for tweaks #1

petehunt opened this issue Dec 16, 2013 · 5 comments

Comments

@petehunt
Copy link

This is great! Had a few ideas for improvement.

  • TodoListItemComponent could have editing be on props instead of state
  • Then in componentDidUpdate() you can only focus if (this.props.editing && !prevProps.editing)
  • In general I don't like it when mixins read from props since they can manipulate the public API of a component. So I would move getBackboneObject() to the user of the mixin rather than the mixin itself.

Just a few ideas... this looks great!

@ssorallen
Copy link
Owner

Thanks for the feedback.

  • Since editing is controlled by the user and changes over time, it seems more appropriate as state. Is that incorrect thinking? Are there performance differences for state vs. props?
  • Good point about the Backbone Mixin. I will change that and have the target object implement either getModel or getCollection.

@petehunt
Copy link
Author

Right, I would just move the state to the owner component and have It take the value of the id of the item being edited (or null)

@ssorallen
Copy link
Owner

Gotcha. I'll try that out.

ssorallen added a commit that referenced this issue Dec 16, 2013
As mentioned in #1, it's confusing to modify a component's API in a
mixin. Let the mixin assume the component will implement a method named
`getResource` that returns an object with `Backbone.Events` mixed in.

Fixes #2.
@ssorallen
Copy link
Owner

@petehunt I looked at your suggestion again and am wondering what advantage moving editing to a prop offers. Is that because only one item can be in edit mode at once, and so the list should own that state?

@petehunt
Copy link
Author

exactly

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

No branches or pull requests

2 participants