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

[IDP] Input mask component #4884

Merged
merged 47 commits into from
Nov 9, 2022
Merged

Conversation

jonathanbobel
Copy link
Contributor

@jonathanbobel jonathanbobel commented Jul 27, 2022

Input Mask component

Federalist Storybook - Input Mask
Federalist Content Site - Input Mask

Description

Code for Input Masks - making it easier for users to fill out input content that is easily attached to a regex pattern

  • Creates an array from all inputs with class="masked", placeholder and pattern attributes
  • Code will look to pattern attribute to determine maxlength to limit input
  • Creates mask overlay that when typing removes the characters and replaces them with the true input value
  • Can be used in conjunction with validation, as it doesn't alter the actual contents of the input

Additional information

Patterns include

  • SSN
  • US Telephone Number
  • US Zip Code

This code can also be extended to additional Input Masks if the user is familiar with regex

@jonadecker
Copy link
Contributor

jonadecker commented Jul 28, 2022

In the Federalist preview, the phone is [screen]reading as invalid data. This isn't happening on SSN or ZIP so I don't think it's an onFocus issue.

I'm also wondering whether this script allows you to choose not to display pre-fill text? Our patterns advocate for not displaying pre-fill text because it usually disappears on entering an input field. That isn't true in this case, so I'm not sure it's a problem; it's just something we need to at least mention in the supporting patterns.

I do think we should also show hint text in the preview, even if the mask is displaying pre-fill values in the inputs, both as an affordance and to align with pattern writeups.

Copy link
Contributor

@mejiaj mejiaj left a comment

Choose a reason for hiding this comment

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

Overall the component functions well. Added some suggestions and questions below (mostly for code style and readability).

packages/usa-input-mask/_index.scss Show resolved Hide resolved
packages/usa-input-mask/src/styles/_usa-input-mask.scss Outdated Show resolved Hide resolved
packages/usa-input-mask/src/styles/_usa-input-mask.scss Outdated Show resolved Hide resolved
packages/usa-input-mask/src/styles/_usa-input-mask.scss Outdated Show resolved Hide resolved
packages/usa-input-mask/src/styles/_usa-input-mask.scss Outdated Show resolved Hide resolved
packages/usa-input-mask/src/index.js Outdated Show resolved Hide resolved
packages/usa-input-mask/src/index.js Outdated Show resolved Hide resolved
packages/usa-input-mask/src/index.js Outdated Show resolved Hide resolved
packages/usa-input-mask/src/index.js Outdated Show resolved Hide resolved
packages/usa-input-mask/src/index.js Outdated Show resolved Hide resolved
@mejiaj mejiaj requested a review from amyleadem August 4, 2022 16:05
@jonathanbobel
Copy link
Contributor Author

jonathanbobel commented Aug 4, 2022

In the Federalist preview, the phone is [screen]reading as invalid data. This isn't happening on SSN or ZIP so I don't think it's an onFocus issue.

I'm also wondering whether this script allows you to choose not to display pre-fill text? Our patterns advocate for not displaying pre-fill text because it usually disappears on entering an input field. That isn't true in this case, so I'm not sure it's a problem; it's just something we need to at least mention in the supporting patterns.

I do think we should also show hint text in the preview, even if the mask is displaying pre-fill values in the inputs, both as an affordance and to align with pattern writeups.

@jonadecker =>
Addressed the hint and the invalid data screen reader issues.

The javascript looks for the placeholder in order to render the mask and determine the input length, so if the user didn't want that functionality, they could use the existing text input component. We might have to update guidance to reflect this difference.

mejiaj
mejiaj previously requested changes Oct 11, 2022
Copy link
Contributor

@mejiaj mejiaj left a comment

Choose a reason for hiding this comment

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

Very nice! Component looks and works well. Also tested it in voiceover and didn't notice any issues. Added some suggestions below.

You also mentioned it can be used in conjunction with character count   — does that work and is it recommended?

packages/usa-input-mask/src/index.js Outdated Show resolved Hide resolved
packages/usa-input-mask/src/index.js Outdated Show resolved Hide resolved
packages/usa-input-mask/src/index.js Outdated Show resolved Hide resolved
packages/usa-input-mask/src/index.js Outdated Show resolved Hide resolved
packages/usa-input-mask/src/index.js Outdated Show resolved Hide resolved
packages/usa-input-mask/src/index.js Outdated Show resolved Hide resolved
packages/usa-input-mask/src/index.js Outdated Show resolved Hide resolved
packages/usa-input-mask/src/index.js Outdated Show resolved Hide resolved
packages/usa-input-mask/src/index.js Outdated Show resolved Hide resolved
@mitchmoccia
Copy link
Contributor

@mejiaj have a couple more comments to address but made some great progress... To answer your question about character count and input mask... I confirmed with Dan that we should not use character count with input mask, only free text fields. Thanks and more to come!

@thisisdano thisisdano changed the title Jb inclusive patterns input mask [IDP] Input mask component Nov 2, 2022
@thisisdano thisisdano changed the base branch from develop to idp-staging November 2, 2022 05:16
@thisisdano thisisdano dismissed mejiaj’s stale review November 2, 2022 05:19

Addressed comment

@thisisdano
Copy link
Member

@mitchmoccia I'm having trouble getting this to work for me. Did something go wrong?

@amyleadem
Copy link
Contributor

amyleadem commented Nov 8, 2022

Just reviewed the interaction and behavior of the component. Looking good on my tests so far! I had a couple of notes about giving users more feedback and instructions. At this stage of the game, I expect these might need to be enhancements to add later, but I wanted to throw them out there in case there is time to consider them.

Error handling

It would be helpful to give some kind of feedback for when a character is entered that doesn't fit the acceptable pattern. This feels especially important when using assistive technologies to navigate this component. Right now adding an item that doesn't fit just returns silence.

More detailed instructions

Additionally, more detail about what kind of entry is acceptable would be helpful. Adding something to indicate the number of characters expected along with explicit directions for the expected character pattern. So social security could have something as simple as "Expected input: Nine numbers. For example, 123 45 6789", and the alphanumeric example could have something like "Expected input: Six characters with alternating letters and numbers. For example, A1B 2C3." I expect this could be added either to usa-hint or as an aria-label on the input.

@thisisdano
Copy link
Member

thisisdano commented Nov 9, 2022

I approve this for release, but I think this component needs fast follow-up. Here's an issue that outlines my current concerns: #5025

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

Successfully merging this pull request may close these issues.

6 participants