Skip to content

Conversation

@asudoh
Copy link
Collaborator

@asudoh asudoh commented Oct 25, 2012

Includes the following changes:

  • Moved code of following controllers to new mixins, by renaming those and removing the inheritances of parent controllers. The original controllers have become just declare of ModelRefController and below new mixins:
    • EditModelRefController -> EditModelRefControllerMixin
    • StoreRefController -> StoreRefControllerMixin
    • EditStoreRefController -> EditStoreRefControllerMixin
    • ListController -> ListControllerMixin
    • EditStoreRefListController -> EditModelRefListControllerMixin and EditStoreRefListControllerMiixn
  • holdModelUntilCommit option support in commitCurrent() (Now in EditModelRefListControllerMixin), with test cases for this new feature
  • resetCurrent() support in EditModelRefListControllerMixin, which only resets the currently selected list item, with test cases for this new feature

@ghost ghost assigned edchat Oct 25, 2012
@edchat
Copy link
Owner

edchat commented Oct 26, 2012

Hi Akira, thanks for working on this. I was hoping we could get down to only a few mixins, like one each of these:
Edit, Store, and List.
And for example having "Edit" and "Store" would mean we would not have to have a EditStoreRefControllerMixin.
Or having "Edit", "Store" and "List" would remove the need for a EditStoreRefListControllerMiixin.
So I was hoping for 3 mixins instead of 6.

@asudoh
Copy link
Collaborator Author

asudoh commented Oct 26, 2012

@edchat Thank you for your feedback. One of the reasons I ended up with many mixins was there are pieces of code that are relevant only for particular combination, for example, the code sending currently selected model to store in commitCurrent(), that's relevant only for Edit/Store/List combination. Given having less mixins seem more desirable, I'll try it, probably by moving such piece of code to Store for example.

@asudoh
Copy link
Collaborator Author

asudoh commented Oct 27, 2012

Reduced 3 mixins as discussed.

@edchat
Copy link
Owner

edchat commented Oct 29, 2012

Hi Akira, thanks for making those updates. In order to make it easier for us to discuss this, I am going to take these updates and create a new branch. I will call it "mixins", and then I will start trying to update some of the tests to see how this works in practice. I don't want to merge it into master until we are confident that this will be the new direction.

@asudoh
Copy link
Collaborator Author

asudoh commented Oct 30, 2012

Hi Ed, sounds good, it makes sense.

@edchat
Copy link
Owner

edchat commented Nov 1, 2012

Hi I have setup this branch with your code, and some of my own changes.
https://github.com/edchat/dojox_mvc/tree/mixin

I will close this pull request, and open a new issue to track this work. I will copy the previous comments from here into the new issue.

@edchat edchat closed this Nov 1, 2012
@edchat edchat reopened this Nov 1, 2012
@edchat
Copy link
Owner

edchat commented Nov 1, 2012

sorry I wanted to see if I could reopen the issue, without reopening the pull request, I guess not.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants