Refactor branch #1134
Refactor branch #1134
Conversation
Adds an onBeforeSelect callback to allow intercepting a selection. Ensures access to both the prospective new selection and the current selection.
* fix_issue_453: Fix test to check against ui-select-offscreen class, not ng-hide Update to move search box off screen Don't hide select when SearchEnable is false angular-ui#453 Conflicts: examples/bootstrap.html
* limit_multi_select: Added test to check multiple limit Limit the number of selections allowed in multiple mode
…actor_and_tidy Conflicts: test/select.spec.js
…ashed commit) Squashed commits: [25d355a] Hacking!!!
|
This diff is way too large to review. @cdjackson I think you may have to just develop on a branch in here |
|
Fixes #848 |
|
@ProLoser currently I've changed the callbacks to use controller stubs (it possibly needs more modularising yet, but...). The reason I went down this route is that for some parts of the selection where overriding the default implementation is expected, I think it's useful to support promises where async operations is likely (eg the beforeDropdownOpen where the dev might want to populate the list from a server or something). Thus, if the dev wants to override these callbacks for basic callback functions, it's very simple, and there's a clean interface that provides a good level of functionality. If they want to do something fancier, then they can override a lower level method, but then their a little more on their own as this won't be documented. I've also split the tagging out into a separate directive - I wouldn't call it refined, but it functions ok from my initial tests (it needs some more work). I had to add the KEY.MAP back into the code to cover tagging - I could have put it into the tagging directive since this is the only place it's currently needed, but it means other directives wouldn't be able to use it... Previously this was in the global space which I didn't like. I might revisit this as a trawl through the issues list tonight revealed a problem in this area with keycode mapping... There's still some work to do on this, but I thought I'd throw out an update... I'm not sure how you want to proceed? It needs more tests, and it needs docs, and probably more of a tidy, It would be good if we could get a few more people commenting/testing this - my view is it would be good to get it merged, close all outstanding issues and request people resubmit if needed (as we head toward 500 issues and 180 PRs, it's unmanageable). |
|
@cdjackson I understand a desire to support promises, but that arrises from treating the list of items as something private and very precisely controlled. If you make these items public, then the user can modify this collection anytime they wish, they can do ajax and just update the collection anytime they want, the support for promises because superfluous. Again, I still recommend not using before and after callbacks and simply demonstrating how to override extremely simplified functions. |
|
For now to make the code easier to review, can you remove the dist folder? It's just bloating this PR tremendously, we can deal with it after all the src is reviewed. |
|
I also feel that the repeater logic needs to be removed. It just bloats and complicates everything. Instead of allowing people to define the template for one row and handling the repeating for them, just move the logic up one level and let people define the entire dropdown container. |
True, but they might still want this loading to be triggered when the dropdown is opened... Anyway, I appreciate there's many ways to achieve this so I'll take another look - it might be another few days (other things to do and all that :) ). I'll remove the dist files now though. |
|
If you look at how I do it in ui-mention, you can override the 'findChoices' method and set your own loading flag and use that alongside the list of choice,s |
| @@ -0,0 +1,190 @@ | |||
| angular.module('ui.select.tagging', ['ui.select']) | |||
| .directive('uiSelectTagging', | |||
| ['$parse', '$timeout', function () { | |||
There was a problem hiding this comment.
You shouldn't be using this injection syntax.
Continuation of #1108 refactor. Will try to give feedback here.