Skip to content
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

✨ Support input masking four date formats #20383

Merged
merged 2 commits into from Jan 17, 2019

Conversation

cvializ
Copy link
Contributor

@cvializ cvializ commented Jan 16, 2019

This PR implements support for the following named masks to amp-inputmask:

  • date-dd-mm-yyyy
    • Supports dd/mm/yyyy formatted dates
  • date-mm-dd-yyyy
    • Supports mm/dd/yyyy formatted dates
  • date-mm-yy
    • Supports mm/yy formatted dates
  • date-yyyy-mm-dd
    • Supports ISO yyyy-mm-dd formatted dates

Known Issues

  • deleting/inserting in the middle of the mask causes unexpected behavior

Fix date separators not appearing automatically
Copy link
Contributor

@nainar nainar left a comment

Choose a reason for hiding this comment

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

Names lgtm as documented in slack

Copy link
Member

@Gregable Gregable left a comment

Choose a reason for hiding this comment

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

validator changes look good

@cvializ cvializ merged commit bb4b411 into ampproject:master Jan 17, 2019
@@ -46,6 +46,10 @@ tags: {
"payment-card" # A named mask followed by
"\\s" # a single whitespace
"[^\\s]+)" # followed by one or more non-whitespace characters.
"|([^\\s]+\\sdate-dd-mm-yyyy|date-dd-mm-yyyy\\s[^\\s]+)"
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't believe it's necessary to repeat the pre and post amble for whitespaces for each named mask. Instead these new ones could be put with payment-card. Please verify with tests of course.

E.g.

        "([^\\s]+"          # One or more non-whitespace characters before
        "\\s"               # a single whitespace
        "(?:"               # non capturing group of one of the following:
        "date-dd-mm-yyyy|"  # a named mask or
        "date-mm-dd-yyyy|"  # a named mask or
        "date-mm-yy|"       # a named mask or
        "date-yyyy-mm-dd|"  # a named mask or
        "payment-card"      # a named mask
        ")"                 # end of non capturing group.
        "|"                 # Or (to cover the other case):
        "(?:"               # non capturing group of one of the following:
        "date-dd-mm-yyyy|"  # a named mask or
        "date-mm-dd-yyyy|"  # a named mask or
        "date-mm-yy|"       # a named mask or
        "date-yyyy-mm-dd|"  # a named mask or
	    "payment-card"      # a named mask
        ")"                 # end of non capturing group. Followed by:
	    "\\s"               # a single whitespace
	    "[^\\s]+)"          # followed by one or more non-whitespace characters.

I also noticed this does not catch the attribute value of payment-card date-dd-mm-yy. The single whitespace should become \\s+ # one or more whitespaces, followed by. Probably want to add a test case for this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the suggestion, this should help prevent mistakes when adding future named masks. I'll make this change in a followup PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks! To be clear on the case it did not catch, the example has two whitespaces between payment-card and date-dd-mm-yyyy.

Gregable pushed a commit that referenced this pull request Jan 22, 2019
noranazmy pushed a commit to noranazmy/amphtml that referenced this pull request Mar 22, 2019
* Support input masking four date formats

Fix date separators not appearing automatically

* Add missing dependency check config
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants