✨Add support for mask-trim-zeros attribute on amp-inputmask.#22175
✨Add support for mask-trim-zeros attribute on amp-inputmask.#22175cvializ merged 3 commits intoampproject:masterfrom
Conversation
examples/masking-trim.amp.html
Outdated
| <label>CPF: <input name="cpf" mask="000.000.000-00" placeholder="000.051.271-20" mask-trim-zeros="0"></label> | ||
| <input type="submit"> | ||
| </form> | ||
| <p>It should displays <strong>000.051.271-20</strong>, but is showing <u>005.127.120</u></p> |
There was a problem hiding this comment.
displays -> display, remove "but ..."?
There was a problem hiding this comment.
I'll remove that line since it was just explanation for a repro case
| config['alias'] = 'custom'; | ||
| config['customMask'] = inputmaskMask; | ||
| this.controller_ = this.Inputmask_(config); | ||
| return; |
There was a problem hiding this comment.
Returning early from a constructor feels weird (actually, even having control flow logic in the constructor itself feels weird). I think this would feel a bit more natural if your constructor looked like:
this.Inputmask_ = Mask.getInputmask_(element);
this.element_ = element;
this.controller_ = this.createController_();where you could return early from the createController_ method.
There was a problem hiding this comment.
solid point, I had forgotten it was a constructor when I wrote that : P I'll remove the early return but since the config depends on which controller is created I'll keep it in one method to keep state in one place
| */ | ||
| export function removePrefix(value, prefixes) { | ||
| const longestPrefix = prefixes | ||
| .filter(prefix => value.indexOf(prefix) == 0) |
There was a problem hiding this comment.
Not a huge deal, but would use startsWith from src/string. A little more clear on what it is doing, and also uses lastIndexOf to stop searching immediately if it doesn't start with the other string.
| const prefixes = opts['prefixes']; | ||
| const trimZeros = opts['trimZeros']; | ||
|
|
||
| let processedValue = value; |
There was a problem hiding this comment.
I tend to like to remove special cases, and just go through one code path. I think this should be equivalent?
const trimZeros = opts['trimZeros'] || 0;
const processedValue = value.replace(new RegExp('^0{0,${trimZeros}}'), '').replace(/[\s]/g, '');I think having the code a bit shorter and easier to read outweighs any marginal performance benefits (i.e. from doing a replace that doesn't do anything).
| const prefixes = {}; | ||
| for (let i = 0; i < masks.length; i++) { | ||
| const prefix = getMaskPrefix(masks[i]); | ||
| // The array of subprefixes grows with the factorial of prefix.length |
There was a problem hiding this comment.
Should we print a warning for the developer here?
|
Addressed feedback, PTAL |
Document and add tests for custom alias.
Fixes #21706 (the original document will need to add
mask-trim-zeros="0")✅Adds unit tests, since the integration tests ended up being flaky
📖Renamed
README.mdto fit the convention for the other componentsA small refactor to improve unit-testability.