Skip to content
This repository has been archived by the owner on Apr 12, 2024. It is now read-only.

external removal of input formatters #10168

Closed
wants to merge 1 commit into from
Closed

Conversation

rupe120
Copy link

@rupe120 rupe120 commented Nov 22, 2014

I'm attempting to come up with a solution for this ui.Bootstrap Typeahead bug: angular-ui/bootstrap#2681

An alternative to this pull request is questioning if we really need the formatter which does the toString()

The removeByFnName() would be called from the Typeahead to remove the stringBasedInputTypeFormatter

Thoughts?

@googlebot
Copy link

Thanks for your pull request.

It looks like this may be your first contribution to a Google open source project, in which case you'll need to sign a Contributor License Agreement (CLA) at https://cla.developers.google.com/.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check the information on your CLA or see this help article on setting the email on your git commits.

Once you've done that, please reply here to let us know. If you signed the CLA as a corporation, please let us know the company's name.

@pkozlowski-opensource
Copy link
Member

@rupe120 thnx for the PR. I can see what you are trying to do but the proposed change is not good as it breaks the existing tests (so the existing functionality as well). I think that the problem with the parsers / formatters pipeline is somehow deeper and we should look into it alongside with other issues around NgModelController.

@Narretz @petebacondarwin the real use-case from the typeahead is that currently NgModelController is missing a hook point that would allow us to do some processing when a value in the input changes. What is missing is an equivalent of the ngChange - sth like ngViewChange or similar (registering to the input event is not good since there is already too much magic going on around view change detection - the logic that we don't want to duplicate in custom directives).

I think that we should keep the typeahead use-case in mind while working on the form changes in 1.4 or whenever it happens.

@rupe120
Copy link
Author

rupe120 commented Nov 22, 2014

@googlebot the CLA is now signed

@pkozlowski-opensource You're welcome. I apologize for not running the unit tests. If I can get these changes to work with the tests, should I resubmit, or is this change not something that you are comfortable with? I agree that the real fix should take into account other issues that I'm not aware of, but it seems like this may be an ok bandaid until then.

@pkozlowski-opensource
Copy link
Member

@rupe120 I don't think it makes sense to add tests to the change in the current form, we should rather look at the pb more globally. I think that @Narretz is working on some design docs for the form changes - we should make this public when ready.

@Narretz
Copy link
Contributor

Narretz commented Nov 23, 2014

@pkozlowski-opensource Pete created the forms doc; I think you should be able to edit it too: https://docs.google.com/document/d/1-MhomULgCFOXVsqAGrI7l-cXYMYmJM4BMzgy_anC93o
A real viewchange listener is something I thought about as well.

@pkozlowski-opensource
Copy link
Member

@Narretz awesome. Glad to hear that you were pondering view change listeners.

@Narretz
Copy link
Contributor

Narretz commented Nov 23, 2014

@rupe120 Why can't you remove the formatter from a directive?

@rupe120
Copy link
Author

rupe120 commented Nov 23, 2014

@pkozlowski-opensource Ok sounds good.

@Narretz It was more a matter of reliably removing the correct formatter if there were other's added

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants