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

Allow use of BackboneMixin without arguments; fix controlled components and changing models #18

Merged
merged 21 commits into from
Apr 16, 2014
Merged

Allow use of BackboneMixin without arguments; fix controlled components and changing models #18

merged 21 commits into from
Apr 16, 2014

Conversation

EamonNerbonne
Copy link
Contributor

This PR builds on @markijbema's #15.

It fixes two subtle bugs:

  1. When the model was changed due to a prop change, the event handlers were not rebound properly. This PR rebinds them correctly.
  2. When the model was updated, due to the debounce, controlled components would lose cursor positoin. This PR avoids the debounce for models, and only debounces collections. Note that this still isn't an ideal situation - really we'd want to fix this on the React side of things, such that forceUpdates aren't immediate, because it's just wasteful to re-render all the time (and this isn't the only way to cause re-renders).

It also supports using BackboneMixin without arguments.

markijbema and others added 17 commits March 12, 2014 10:37
this is already checked in the guard
was redundant with documented code
changeOptions should only be declared once
to make style consistent with subscribe/unsubscribe,
which use similar constructs for checking preconditions
This way we will be able to mixin multiple times
also added use strict to ensure this won't happen again
used parameters instead of binding to this. Thanks to @EamonNerbonne for
the suggestion
Also kept createBackboneClass backwards compatible
@markijbema
Copy link
Collaborator

👍

@clayallsopp
Copy link
Owner

This is great - @markijbema did you ever revisit this? #15 (comment)

@markijbema
Copy link
Collaborator

No sorry, didn't get around to doing that yet.

@EamonNerbonne
Copy link
Contributor Author

However, I just did :-)

Behavior is slightly tricky (backwards compat often is, alas): change options are now those customized if available, otherwise the component (spec's) changeOptions, otherwise a default based on whether the model is a collection or just a model.

@EamonNerbonne EamonNerbonne changed the title Fix controlled components and changing models Allow use of BackboneMixin without arguments; fix controlled components and changing models Apr 16, 2014
@markijbema
Copy link
Collaborator

👍 this was exactly what I wanted to do, thanks @EamonNerbonne !

This was referenced Apr 16, 2014
@clayallsopp clayallsopp merged commit 1874223 into clayallsopp:master Apr 16, 2014
@clayallsopp
Copy link
Owner

It is done! Thank you both very much for all the work, apologies for the extraordinarily long delay 👏

@EamonNerbonne EamonNerbonne deleted the fix-controlled-components-and-changing-models branch April 17, 2014 13:34
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.

3 participants