Skip to content
This repository has been archived by the owner on Oct 2, 2019. It is now read-only.

Update model on sort (fixes #974) #1036

Merged
merged 4 commits into from
Apr 27, 2016
Merged

Conversation

homerjam
Copy link
Contributor

No description provided.

@faisalferoz
Copy link

+1

@pgrm
Copy link

pgrm commented Aug 18, 2015

would be great if this could be merged, any updates?

@homerjam
Copy link
Contributor Author

bump

@mrak1990
Copy link

mrak1990 commented Feb 8, 2016

bump, without this fix sortable option not useful

@dmitry-dedukhin
Copy link

Although exists a workaround #974 (comment) when we have more than one ui-select we can't use uiSelectSort:change event since we don't know which ui-select has triggered event.

@wesleycho
Copy link
Contributor

Remove changes to dist, squash and rebase.

@homerjam
Copy link
Contributor Author

Hi @wesleycho sorry I have no idea how to do what you just said. I've reset to the previous commit (without changes to dist) and force pushed. Hopefully this works?

@@ -92,6 +92,8 @@ uis.directive('uiSelectSort', ['$timeout', 'uiSelectConfig', 'uiSelectMinErr', f
}

move.apply(theList, [droppedItemIndex, newIndex]);

scope.$parent.$selectMultiple.updateModel();
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of going through $parent can't we require: ['^uiSelect', '?^uiSelectMultiple'] and access via a ctrls[] param?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've just tried this and the 2nd controller is undefined? I'm not quite sure what to try...

Choose a reason for hiding this comment

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

I think you can require ^ngModel and not ^uiSelectMultiple and call ngModel.$setViewValue(Date.now());

Is it not the only useful thing that updateModel does here? Everything else seems like overhead in this function for what you are trying to achieve.

Copy link

@qraynaud qraynaud Apr 25, 2016

Choose a reason for hiding this comment

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

I tried this and it is working fine :

require: ['^^uiSelect', '^ngModel'],
link: function(scope, element, attrs, ctrls) {
  var $select = ctrls[0];
  var $ngModel = ctrls[1];

  // ...

  $ngModel.$setViewValue(Date.now());

  // ...
}

@homerjam
Copy link
Contributor Author

@user378230 updated with @qraynaud 's suggestions - can confirm it works

@qraynaud
Copy link

@homerjam : you have conflicts to fix, hope it will me merged (& released) soon. I need this working too 👍

@user378230
Copy link
Contributor

@homerjam This LGTM 👍

Will merge once conflicts resolved.

homerjam and others added 2 commits April 26, 2016 14:10
@homerjam
Copy link
Contributor Author

Sorry guys I have no idea what I'm doing, literally. Will this do?

@qraynaud
Copy link

@homerjam : you removed the double ^ on uiSelect. I don't think this is a good idea.

@homerjam
Copy link
Contributor Author

Nuts. Ok. What does that do?
On 26 Apr 2016 3:06 p.m., "Quentin Raynaud" notifications@github.com
wrote:

@homerjam https://github.com/homerjam : you removed the double ^ on
uiSelect. I don't think this is a good idea.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#1036 (comment)

@user378230
Copy link
Contributor

@homerjam can you confirm that the fix works with the change back to ^^?

This needs a squash to a single commit but I can do that as part of the merge. (See https://help.github.com/articles/about-git-rebase/#further-reading if you want to have a go yourself)

@homerjam
Copy link
Contributor Author

Hi @user378230

I can confirm that it works and have since read up on the ^^ and seems a good idea.

I've tried doing the rebase several times but I keep getting conflicts and going round in circles 😞 . Thanks for the link but in this case please can you take over in the interest of getting this done!

Thanks

@user378230 user378230 merged commit 9a40b6f into angular-ui:master Apr 27, 2016
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants