Play nicely with formatters #568
Conversation
|
@gcallaghan Thnx for the PR. It is on my TODO-list to review how the typeahead interacts with formatters / parsers pipelines as I knew that things are not super-well integrated at the moment. Regarding your change - I'm not sure if formatters should be called on each render - I would say that those should be rather invoked while setting $viewValue, don't you think? If you could provide a test for your use case (we will need a test before merging anyway) we could work it out together. Thnx! |
|
You are right. I think I've found a better location to hook into the $formatters/$parsers pipeline. I think we won't even need render() when its done. Now I just have to figure out how to test it. Re: viewValue Unfortunately some versions of the angular documentation are wrong. $setViewValue invokes the $parsers pipeline. the $formatters pipeline is only affected when the underlying model changes programmatically and the $modelValue and model are out of sync. see: here |
|
@pkozlowski-opensource I've added the test and the modified 2 tests to reflect the new behavior. Let me know what you think when you get a chance. Thanks! |
|
@gcallaghan Could you please squash all the commits into one? It is very difficult to review changes across many commits... (or at least I haven't figured out how to do this effectively!) Going quickly over your changes I'm worried that you had to modify existing tests. We shouldn't change the behavior of typeahead to support formatters (at least I hope so). For me all the existing tests should be passing with your change (as those are not using any custom formatters). So if you had to modify a change it would mean that the behavior have changed... which would be undesirable. Anyway, please sqash the commits so I can review it properly. |
|
@pkozlowski-opensource Wow, I just learned a whole lotta' git! Now that those git gymnastics are done it should be easier to review. btw, I share your concern regarding the fact that i had to modify existing tests and thought that warranted a larger discussion, hence the noise on the newsgroup. Anyway, thanks for your attention and feedback! |
| @@ -250,7 +251,7 @@ describe('typeahead tests', function () { | |||
| $scope.states = [{code: 'AL', name: 'Alaska'}, {code: 'CL', name: 'California'}]; | |||
| $scope.result = $scope.states[0]; | |||
|
|
|||
| var element = prepareInputEl("<div><input ng-model='result' typeahead='state as state.name for state in states'></div>"); | |||
| var element = prepareInputEl("<div><input ng-model='result.name' typeahead='state as state.name for state in states'></div>"); | |||
There was a problem hiding this comment.
From my understanding, on an angular input, the value is typically tied to a property with a "primitive" value, i.e. string, int, bool, etc.
There was a problem hiding this comment.
Yes, this is typical, but shouldn't be mandatory. We need to preserve how it used to work (compare it to the <select> directive.
There was a problem hiding this comment.
Yes, but in the select directive the option label is what is displayed much like the value of the input element. However, the underlying model pointer is changed by the selection action in both the select directive and this change. The value, or label, is then updated to represent the current value of the model.
TLDR; I believe this behaves comparatively to the select directive.
There was a problem hiding this comment.
@gcallaghan OK, I need to take a deeper look into it. Can't do this right now but should have a spare hour over the weekend. Thnx for the PR and bearing with me here!
There was a problem hiding this comment.
@gcallaghan I'm sorry but looking into it once again I can definitively say that it is a breaking change that shouldn't be introduced. We need to be able to bind the whole object (and not only it properties) to the model.
In short - you shouldn't need to change those tests in your PR.
|
@gcallaghan I've solved this differently in bb67a97 As you can see it doesn't require changes to the existing tests (apart from one that was truly broken!) and also getting rid of the $render method. I'm going to merge it as soon as #595 is sorted out. Thnx for the PR, it forced me to have a closet look at those topics! |
|
Ah nuts! I was hoping to contribute! ah well. I like your solution better and I learned a ton! Glad it got fixed! And good work getting the tests to run on more browsers. |
|
Sorry @gcallaghan ... I was doing other refactorings and fixed this one as part of other changes... |
I am attempting to use multiple directives on a typeahead but found that the formatters where not being called on render. This change calls the formatters associated with the modelCtrl.
see: http://plnkr.co/edit/jt6NRcNVPQHnLlRm6iFu?p=preview