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

ui-mask improvement #50

Merged
merged 3 commits into from
Jun 21, 2013
Merged

ui-mask improvement #50

merged 3 commits into from
Jun 21, 2013

Conversation

pocesar
Copy link
Contributor

@pocesar pocesar commented Jun 14, 2013

for issue #16

  • I exposed the maskDefinitions through a value, that can be extended at will, or by ui-options per element.
  • Fixed a "global" variable leak viewValue (without var) while making the changes to the code.
  • Added more tests, everything is passing on Firefox/Chrome.
  • Added more mask values by default, like only lowercase, lowercase and uppercase and 0-x (being x 0 to 9)

I already signed the CLA

@ProLoser
Copy link
Member

@shaungrady do you want to review/merge this one?

@ProLoser
Copy link
Member

@pocesar does this address #27?

@ProLoser
Copy link
Member

@pocesar also maybe you can throw in one more unit test to confirm that this PR fixes #31

@shaungrady
Copy link
Contributor

Only comment I have is about the additional maskDefinitions—I can see it being common for someone to have a phone mask of "+1 (999) 999-9999". Adding 1 as a definition forces you to specify your own definition. Granted, this is an issue with American bias. Perhaps, though, this calls for the functionality to be added to escape a definition character.

@ProLoser
Copy link
Member

Actually @pocesar i might have to agree with @shaungrady on that. I don't really see the added benefit of having 1-8 masks. It seems oddly specific and only caters to unusual edge-cases. Why should you be allowed any number between 0 and 5? Why not only 5? Why not 5 to 9? What about 4 or 5 or 6?

I'd rather us not causing regressions on more common use-cases (as @shaungrady points out) until we have a solid implementation or fix.

@pocesar
Copy link
Contributor Author

pocesar commented Jun 14, 2013

the biggest use is for dates: 19/39/2999 or 39/19/2999 (that I usually use)

Anyone can easily remove the mask that they want using delete maskConfig.maskDefinitions['1'] for example. I've even added more tests, including tests that were missing to the optional ? char. But of course, it can come without any of those extra masks, and I'll just add mines.

One thing that I didn't include in this commit is the ability to set the model to the $viewValue. I always want my data to be formatted, let the server deal with the 'unformatting', I shall make another push if this get merged depending on how it goes.

That's why I also wanted a "fixed" non mask character modifier that could signal to the directive to skip that number of mask. It could be quoted, like +'1' (999) 999-9999, that would make the 1 'fixed' and make the directive ignore/skip it. there's also another library for jQuery called meiomask, it has some default mask values, like credit cards, phone numbers, dates, etc are a bit different from masked.input

@shaungrady
Copy link
Contributor

There's a fine line that exists between masking and validation. When you start talking about dates, that's when it starts to cross over to validation—something this directive isn't designed to handle. When you start offering those kinds of rudimentary validation use-cases to users, they're going to wonder why it's not more sophisticated (e.g., why it only works on individual characters). There are many better, purpose-built solutions for doing client-side validation of input data.

That aside, I agree that it would be nice to allow the user to specify that the model reflect the formatted view value. As for "fixed" non-mask characters, I think "escaping" is a term more would be familiar with, and I'd advocate for a simpler, more familiar backslash () escaping of a mask character.

@pocesar
Copy link
Contributor Author

pocesar commented Jun 14, 2013

I agree. And yes, escaping is the correct term. I'll remove the extra masks from the commit, and commit just the exposed configuration part.

@ProLoser
Copy link
Member

Does your PR support dynamic masks? Is it possible to do ui-mask="{{someMaskString}}"?

@pocesar
Copy link
Contributor Author

pocesar commented Jun 14, 2013

nothing in that matter has changed @ProLoser it still support dynamic stuff, including the new configuration

}
iElement.bind('blur', blurHandler);
iElement.bind('mousedown mouseup', mouseDownUpHandler);
iElement.bind('input keyup click', eventHandler);
Copy link
Member

Choose a reason for hiding this comment

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

What is the purpose of binding to anything other than input?

Copy link
Contributor

Choose a reason for hiding this comment

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

Cross-browser compatibility. The input event isn't supported below IE9, and IE9 doesn't fire the event on backspace. Additionally I found that the input event wasn't providing a which value in the event object. I don't recall if this was for all browsers or some, but I wrote about it in a couple of the code comments.

Copy link
Contributor

Choose a reason for hiding this comment

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

The click listener is used purely for nudging the caret leftward to the nearest non-mask character if the user placed the caret to the immediate right of a mask character. The reasoning behind this is that a user is arguably more likely to backspace a character on a filled input when clicking into it, so placing the caret in front of a deletable char makes the most sense.

@ProLoser
Copy link
Member

@shaungrady I will defer the decision to merge or moderate this directive to you. I can't look into the nuances of this atm so I am not going to merge this in myself. If you think this PR as-is cannot be merged in but perhaps a subset of fixes can, maybe you can outline them so that @pocesar might create a PR we can readily merge in while we review the other more contentious issues.

shaungrady added a commit that referenced this pull request Jun 21, 2013
@shaungrady shaungrady merged commit 129c365 into angular-ui:master Jun 21, 2013
@gustavohenke
Copy link

@pocesar
This PR fixed passing formatted values to the server? If yes, how to..?

@pocesar
Copy link
Contributor Author

pocesar commented Oct 14, 2013

@gustavohenke no, for that, you might want to apply another directive that returns the viewValue instead of modelValue, as per (my) question on stackoverflow: http://stackoverflow.com/questions/17116730/how-to-access-the-ngmodelcontroller-from-inside-the-controller-without-a-form-a

@gustavohenke
Copy link

That would work for simple scope properties; however, I have filters[ filter.id ].
I know I could use $parser(...).assign(...) or $scope.$apply(), but they trigger the digest cycle and that breaks the value.

@shaungrady
Copy link
Contributor

You can access the viewValue of any form input by doing the following:

HTML:

<form name="demo">
  <input name="i_am_masked" ng-model="phoneNumber" ui-mask="(999) 999-9999">
</form>

Controller:

$scope.$watch('phoneNumber', function() {
  // Whenever i_am_masked has a valid value that satisfies the mask definition, uiMask
  // will set the value of "phoneNumber" to the unmasked (unformatted) value of the input.
  // So that's the best time to get the $viewValue of the input from the ngModelController.
  $scope.formattedPhoneNumber = $scope.demo.i_am_masked.$viewValue;
})

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

Successfully merging this pull request may close these issues.

None yet

4 participants