-
-
Notifications
You must be signed in to change notification settings - Fork 7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Enhancement: Numeral Mask + External Lib Formatter For v-text-field #2235
Conversation
Thank you for the very thorough PR. This is quite a bit to digest so I've tagged the entire team to take a look. |
- Improve codes - Improve internal comments - Discovered and fixed a small and hardly noticeable bug on mask change code. The bug was prior to this PR
src/mixins/numeral-formatable.js
Outdated
let parsed = [] | ||
let leadingZeros = true | ||
|
||
for (const digit of text) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
could be text.replace(/^0+/, '')
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice one 👍
src/mixins/numeral-formatable.js
Outdated
} | ||
|
||
parsed = parsed.join('') | ||
return parsed[0] === '.' ? '0' + parsed : parsed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it assume that decimal point is always a dot?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's always a dot in this.lazyValue
.
src/mixins/numeral-formatable.js
Outdated
}, | ||
removeNonNumeral (text, preserveDecimal = false) { | ||
if (text == null) return '' | ||
return preserveDecimal ? text.replace(new RegExp(/[^\d.]/, 'g'), '') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it assume that decimal point is always a dot?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's always a dot in this.lazyValue
.
src/mixins/numeral-formatable.js
Outdated
parseFloat (text) { | ||
text = this.removeLeadingZeros(text) | ||
|
||
const decimal = text.lastIndexOf('.') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it (and 3 lines below) assume that decimal point is always a dot?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's always a dot in this.lazyValue
.
src/mixins/numeral-formatable.js
Outdated
}, | ||
// Correction on arbitrary value entered externally | ||
attemptNumeralCorrection (text) { | ||
const decimalPoint = text.lastIndexOf('.') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it assume that decimal point is always a dot?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's always a dot in this.lazyValue
.
src/mixins/numeral-formatable.js
Outdated
oneChar (char, singleChar = '.') { | ||
return char ? char[0] : singleChar | ||
}, | ||
absVal (value) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
absVal
may be misleading name for function that additionaly restricts the max value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's enhancing Math.abs()
. What could be a better name, anyway?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
THB I'd jsut use simply parseInt(value, 10)
(of whatever the var name is) instead of this.absVal(value)
. Why?
-
why anyone should provide negative numbers for block length or precision? And even if someone provide -3 for precision then probably because he want such a precision (to get only numbers like 1000, 2000, not 1234, not 1234.56) and instead he will get 1234.567
-
if someone will provide precision > 20 and we will call
toFixed
with this value - there will be clear message saying that precision > 20 is not supported, if we only limit it silently to 20, there will be no any message, and the precission will be 20 instead of intended >20. This first of all might lead to confusion and second - I bet it would be a very very rare case
Maybe I had more arguments, but was distracted and forgot. Anyway summing up - I'd just remove the method and use parseInt instead.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- This is a good feature. I think it could be made available next. Or should it be in this PR? I'm gonna need some time for this.
- I think the limitation can be removed because the calculations don't use
parseInt()
orparseFloat()
at all, what more thetoFixed()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Replacing this.absVal()
entirely with parseInt(value, 10)
may not work because the later may return NaN. this.absVal()
is needed to ensure that NaN is treated before proceeding.
I will proceed with item (2), but put item (1) on hold for now. this.absVal()
may later be gone. Thank you for the suggestions.
src/mixins/numeral-formatable.js
Outdated
|
||
// Whether it's an integer or a fractional where abs(value) >= 10 | ||
if (!this.precision || | ||
(String(this.lazyValue).length > this.precision + (this.sign ? 3 : 2))) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is the meaning of this 2/3 constants?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The right side of the equality uses this.precision
.
When there's a sign symbol, we need to add 1 on the right side to match the length in this.lazyValue
.
When the value equals or is more than 10.0, we need to add 2 on the right side to include 1.0 or 2.0 etc. to match the length of this.lazyValue
.
All this is because the fall through codes only adjust the caret in situations when we need to maintain a minimum digit count. That is, it could be between 0.0000 and 9.9999. Otherwise, 0.0000 would become 0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And to control the caret for the purpose of natural user experience. TBH, it's challenging to tame the caret 😉
src/mixins/numeral-formatable.js
Outdated
let number | ||
if (this.precision) { | ||
number = text.split('') | ||
number.splice(-this.precision, 0, '.') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it assume that decimal point is always a dot?
src/mixins/numeral-formatable.js
Outdated
// The first size and separator element | ||
// of the arrays becomes the default value. | ||
for (const digit of text.split('').reverse()) { | ||
digit !== '.' && masked.push(digit) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it (and 6 lines below) assume that decimal point is always a dot?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's always a dot in this.lazyValue
. The algorithm used for calculation is all based on this.lazyValue
, which holds the actual number.
src/mixins/numeral-formatable.js
Outdated
@@ -0,0 +1,278 @@ | |||
import { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the custom formatter mechanism powerful enough to support also numeral formatter? What I'd like to achieve is to keep core files (maskable, VTextField) simple and treat numeral formatter not as an internal part of text input but as a kinda external lib (same like this lib with phone numbers) but which is just bundled into Vuetify.
If this is possible (I'm not sure if it is as the mixin uses vars like backspace
/delete
which look tightly coupled with VTextField) we could think about modifying API so that mask
would accept only mask string ('##AA') or a function, and Vuetify would have some masks (texts or functions) predefined with possibility to override it with dev's own masks:
:mask="$vuetify.mask.creditCard"
:mask="$vuetify.mask.createNumericMask({decimal: ','})"
:mask="LibphonenumberMask({ country: 'DE'})"
computedValue: {
phoneMask: LibphonenumberMask({ country: this.country })
}
<v-text-field :mask="phoneMask">
All my comments here are just after quick "visual" scan of the code without using it live, hopefully questions I'm askaing are not very stupid ;)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm proposing a design approach in this PR. There could be other approaches and the one you suggested could be a cleaner one. However, I couldn't think of a stronger design that can allow full control over the caret position. To do so, I need to tap on the keystroke event in VTextField and make maskable
compute the correct function to use. (Perhaps, there's another way?)
The external/plugin approach does have some weaknesses. The user experience isn't that natural. But if the end result is justifiable with such weaknesses, then it does solve some of the end-user problems.
Just to elaborate further, I have a half-cooked mixin that attempts to leverage on features provided by Number.prototype.toLocaleString()
and I can show it if there's an intention to pursue with i18n
in VTextField. I have it injected with something like :mask="{ formatter: 'locale-string': locales: 'ar-EG' }"
and even <v-text-field locales='ar-EG'>
. I tried with several locales
and options
, LTR and RTL. Unfortunately, I solved one locales
+ options
pair but broke the others. The character sets displayed very well on the input field. But when it comes to controlling the caret and fulfilling the user experience, I still couldn't find a way through. There's a lot of work before it can be usable. All in all, I couldn't get away from doing the internal hack. I couldn't imagine if it can be done as an external/plugin.
- Remove hard limit of max 20 from options - Allow block size to accept 0 as an added feature to literally mean no digits allowed - Allow block separator to take an empty string as a mean to clear of digit groupings
src/mixins/numeral-formatable.js
Outdated
} else if (digit === '.') { | ||
masked.push(this.decimal) | ||
integer = true | ||
if (!this.maskNumeral(digit, param)) break |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maskNumeral
always returns true
, so this line (as well as return true
in maskNumeral
) can be removed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The return false
is at line 123.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How did I miss it...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
😄
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Few minor code changes and cleanup
src/mixins/numeral-formatable.js
Outdated
}, | ||
removeNonNumeral (text, preserveDecimal = false) { | ||
if (text == null) return '' | ||
return preserveDecimal ? text.replace(new RegExp(/[^\d.]/, 'g'), '') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd do
const re = preserveDecimal ? /[^\d.]/g : /[^\d]/g
return (text || '').replace(re, '')
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dunno. Making it do ''.replace(re, '')
seems kind of silly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
text ? text.replace(re, '') : ''
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm worried if text ?
would resolve to false
for '0' ?
in some browsers, while 0
is totally a valid digit; hence the text == null
. Should I or should I not worry?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think current code is fine.
src/mixins/maskable.js
Outdated
}, | ||
unmaskText () { | ||
if (!this.mask) return text => text | ||
return this.isNumeralFormatter ? text => this.unmaskNumeralText(text) : text => unmaskText(text) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return this.isNumeralFormatter ? this.unmaskNumeralText : unmaskText
should work (check first)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I got carried away with the multi-parameters call pattern 😁
The same goes with line 80
src/mixins/numeral-formatable.js
Outdated
|
||
switch (separator.length) { | ||
case 0: return [' '] | ||
case 1: return this.oneChar(separator, '') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isn't it just separator[0]
?
Which leads to the
return separator.length ? separator : [' ']
solution
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, now I think it's probably better not to validate any of the options at all because it's the dev's responsibility to define it correctly if he wants his codes to run well.
src/mixins/numeral-formatable.js
Outdated
|
||
if (!Array.isArray(size)) return [this.absVal(size)] | ||
|
||
switch (size.length) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return size.length ? size.map(this.absVal) : [3]
src/mixins/numeral-formatable.js
Outdated
} | ||
}, | ||
numeralPrefix () { | ||
return this.options.prefix ? this.options.prefix : '' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this.options.prefix || ''
src/mixins/numeral-formatable.js
Outdated
text = text.split('') | ||
text.splice(-this.precision, 0, '.') | ||
|
||
return this.sign + this.parseFloat(text.join('')) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe stupid question, but what is this function supposed to do?
if I take '3.25' as a text and follow all steps here I get:
line 238: ["3", ".", "2", "5"]
line 239: (assuming precision = 2): ["3", ".", ".", "2", "5"]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
text
is only [0-9]
when it enters parseNumber()
src/mixins/numeral-formatable.js
Outdated
const decimal = text.lastIndexOf('.') | ||
const fraction = this.getFraction(text.substr(decimal + 1)) | ||
|
||
return text.substr(0, decimal) + '.' + fraction |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isn't it (all this function) just Number(text).toFixed(this.precision)
? (which makes this method unnecessary as it's only used in one place)
Number('0003.2').toFixed(2) === '3.20'
Number('.2').toFixed(2) === '0.20'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same rationale.
src/mixins/numeral-formatable.js
Outdated
return this.sign + this.parseFloat(text.join('')) | ||
}, | ||
parseInt (text) { | ||
return this.removeNonNumeral(this.removeLeadingZeros(text)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return this.removeNonNumeral(text).replace(/^0+/, '')
, and remove removeLeadingZeros
(see comment in parseFloat
)
And maybe remove this function as it's used only in one place and is short enough to inline it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same rationale above.
src/mixins/numeral-formatable.js
Outdated
val = val.substr(i) | ||
} | ||
|
||
const negative = val && val.length > 0 && val[0] === '-' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
const negative = val[0] === '-'
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm.. so index out of bound is not an exception in JS
src/mixins/numeral-formatable.js
Outdated
integer = integer.length ? integer : '0' | ||
if (!this.precision) return integer | ||
|
||
return decimalPoint < 0 ? integer + this.getFraction('') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
similar to parseFloat ()
below I think you can also use here built in toFixed()
function
Probably
return Number(this.removeNonNumeral(text)).toFixed(this.precision)
or something similar (and after this change and change in parseFloat()
getFraction
could be removed)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, it's a design choice I made. Originally, I used the built-in parseInt()
and parseFloat()
everywhere (was why last time you see the hard limit of max 20 for options). Then I dropped them because it's only a computation limit, whereas the input field is free from such limitation. So an app may not want such limitation perhaps because it's not meant to be computed.
src/mixins/numeral-formatable.js
Outdated
@@ -122,6 +122,9 @@ export default { | |||
if (isNaN(parsed)) return 0 | |||
return Math.abs(parsed) | |||
}, | |||
numeralZero () { | |||
return this.addPrefixSuffix(this.precision ? '0.' + this.getFraction('') : '0') | |||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should probably be a computed property so it can be cached.
- Remove unneeded functions. - Rename function arguments. - More caret adjustment when block size=0.
Can the tables the OP showed be added to the Mask Legend Data Table in https://vuetifyjs.com/components/text-fields ? I found this page after I got confused on how to add a custom mask to my text fields, maybe more people don't get how to create their own at first glance... Regards, DAVI |
I imagine they will be once this PR is merged. This is all new API though so it doesn't apply yet. |
Forecast how long this feature will be available? I really need this feature. |
Resolves conflict
+ Removed duplicate definition of lazySelection from VTextField that is already defined in maskable.js + Modified VForm.spec.js to pass yarn test -u. I don't know why it failed because I didn't write any test script that would likely cause the failure. Also I didn't touch the VForm sources.
Codecov Report
@@ Coverage Diff @@
## dev #2235 +/- ##
==========================================
- Coverage 83.66% 79.82% -3.84%
==========================================
Files 145 131 -14
Lines 3348 3385 +37
Branches 1057 1103 +46
==========================================
- Hits 2801 2702 -99
- Misses 398 466 +68
- Partials 149 217 +68
Continue to review full report at Codecov.
|
Please don't change lockfiles unless it's related to your PR |
any news on this PR? |
@jvbianchi There is a milestone assignment for 1.1, we will evaluate this then. |
Ultimately, this PR is way beyond the scope of what I want to support in the core of Vuetify. I would much rather allow for easier integration/provide steps for integrating with a pre-existing library for more advanced functionality. Double this with the fact that the author of this has disappeared, I will be closing this. |
@johnleider I start to get annoyed by your attitude. The PR is actually brilliant, we are using Vuetify for our internal ERP and you need to understand that if you want to provide a components library your mindset needs to be open enough to accept ideas from us (the developers working with your framework). It is the same problem of other components (see the non-flexibility of the drawer or the tab component). Have you wonder why @azaars disappeared? Come'n dude, we are here to enhance Vuetify and make it a better framework, so be more open to accept ideas such this one, which I personally think is great. Now you drop it, so how am I supposed to have proper number formatting into an input box?? |
We accept ideas all the time @raffaeu. I am the creator but it's not just my framework. We have a team of developers that help guide the direction and progression of the project. Not every decision we make is going to please every developer, and not every decision we make is going to be the right one. At the end of the day, I think you are misconstruing the reasons for closing this PR. Let's take This also applies to many other areas of the framework. It is a balance of providing useful functionality to the developer, while also giving them the freedom (and providing examples) of how to use it with other plugins or to create their own. Take #3013 for example. The PR was great, but the direction the team was taking with v1.1 was going to make a lot of it incompatible. Yes, I closed the PR, but we brought @sorinsarca into the collaborators to help bring some of those features to the next minor release. This is the same thing that we did with @azaars before he went mia. Thank you for taking the time to provide feedback and I hope that I have been able to better explain the reasoning for doing things the way we do. |
@johnleider I agree with @raffaeu in some points. It's a very important feature. This is not a validation feature. Vuetifyjs does not allow masking of price (money) field in . External money masquerade libraries do not work well with Vuetifyjs. Almost all systems need this feature. You should review the need to do this. |
@renatosistemasvc The masking system will continue to be improved upon and we will make sure that integrating more advanced features from other libraries is clear and documented, similar to the validation. |
Here are two examples of external libraries working with vuetify vue-the-mask: https://codesandbox.io/s/4x8vrmq5yx |
As an argument against relying on 3rd party solutions, v-money has the following issue:
|
Fair enough. This all will be much easier to manage once v1.1 comes out. |
In the end I am using vue-the-mask, which seems to "work", but MMMV. |
vue-the-mask, like Vuetify's v-text-field built in mask, requires the masked format. I am on day 3 of trying to limit my v-text-fields to only a floating point number. |
Awe man, I've been keeping an eye on this PR for a few months to fix our band-aid solution during prototyping. I've tried vue-the-mask and v-money but ended up with vue-numeric, it's the only one that allows us to make it look similar to vue-text-field but the validation is not integrated nicely. are there are suggestions for the time being on how to integrate a library with v-text so that the currency input looks & behaviour the same way? |
This is something we will look at integrating into the docs, similar to what we did with vuelidate and vee-validate. |
You may also try using my implementation: Unfortunately I have not been able package this up in to a working stand-alone npm package/module. It would be awesome if anyone can help me find out how to package my repo as a standalone module! |
Please, is there any estimate for currency mask on text-fields? |
v-money seems to be working fine https://codesandbox.io/s/5y55k1klll |
Yes, I tried it too, but no computed values are displayed in masked fields (just when we type into). |
Lets not clutter up this issue any more since it's closed. If you want to continue a discussion about this, join us in chat. |
Enhancement: Numeral Mask + External Lib Formatter For v-text-field
This PR is an attempt to enhance
v-text-field
component so that it can provide the following missing but highly desirable masking/formatting features:Design Approach
The existing
mask
property allows developers to specify 2 data types:The existing mask features work on
mask
of typeString
. This PR extends the new features by leveraging on both types. As such it allowsv-text-field
component to handle the following mask cases:mask="credit-card"
mask="numeral"
:mask="{ formatter: 'numeral', prefix: '$', precision: 4 }"
mask="(##) - (AA)"
:mask="{ formatter: function () {}, myOption1: 23, myOption2: '%' }"
Numeral API
Example of Numeral Mask Options
External Library API
formatter
option must be a callback that returns a formatted String that will be shown in the input field.Example of External Library Plugin
This example uses libphonenumber-js, a JavaScript implementation that format phone numbers based on Google's famous libphonenumber library, but does not depend on Google Closure library.
Steps to produce:
npm install libphonenumber-js
.formatter
property specifying a formatter Function from libphonenumber-js. Example shown below.mask
property.Example Formatter Function
Example Vuetify Template
Resolves #2138