CMSgov / design-system

Open source design and front-end development resources for creating Section 508 compliant, responsive, and consistent websites.
https://design.cms.gov
Other
313 stars 86 forks source link

Additional currency field options and composability of masked inputs #514

Closed pwolfert closed 2 years ago

pwolfert commented 4 years ago

Background

I've just come across the second instance in our apps where we want the currency masked-input field to only allow positive integers (no negative money and no decimals). Since it's a use case that has come up a couple times, I'm wondering if that would be a generally useful thing in the design system.

Problem

I have a masked TextField (mask="currency") that also needs to strip out hyphens (so we only have positive money) and drop decimals (for situations where we only want whole-dollar amounts). My solution so far has been to:

  1. Apply additional unmasking onBlur that runs after the masked TextField's own unmasking transform
  2. Save that unmasked value (an integer)
  3. Feed it back to the masked input via <TextField value={thatUnmaskedValue} ... and allow it to apply its currency masking to the bare positive integer

So the problem is that we have additional masking requirements for certain masked fields that aren't supported by the existing set of masks but might also make use of the existing masks. I know similar requirements have popped up in other apps. It would be nice to have a way to either specify custom behavior for masking and unmasking or to have more masking options that are composable or at least that we could select à la carte.

Possible solutions

  1. Provide a way to provide our own functions for masking and unmasking
    • We'd want to have a way to reference the original functions so people don't have to re-implement the same behaviors if they want to format as currency and strip decimals
  2. Provide some sort of mechanism for composing different premade transforms together.
    • This could be implemented like Option 1
    • This could be done with additional props or an options prop (but this wouldn't be really "composable"
    • This could be done in a more React-y sort of way with components, but I haven't dreamt up what this would look like yet

These two options aren't really distinct from each other. I'm leaning towards a solution where we allow custom functions but have a recommended pattern for composing transform functions together. Maybe it could even be allowing an array of functions...but that could over-complicate things. Maybe there's a guide for using the existing mask functions.

I'm imaginging something like this:

<TextField
  mask={Mask.MASK_CURRENCY}
  unmask={value => numberToPositiveInt(Mask.UNMASK_CURRENCY(value))}
/>

// And `mask` would be backwards compatible by allowing strings for the old masks
bernardwang commented 4 years ago

I really like this! Adding custom functions is very doable, and I think we should definitely rewrite our masking logic with a recommended pattern. It would also be really nice to be able to use the existing masks. One question, in the last example would also want to wrap the MASK_CURRENCY result with a custom function too?

So something like this:

<TextField
  mask={value => negativeStringToPositiveString(Mask.MASK_CURRENCY(value))}
  unmask={value => numberToPositiveInt(Mask.UNMASK_CURRENCY(value))}
/>
bernardwang commented 4 years ago

Related topic, I'm working on a ticket about native input attributes like max/min, maxlength/minlength, pattern on the <TextField>. Currently these attributes aren't very useful because most of our React forms don't use native HTML form logic.

Since we currently dont provide a React alternative to those native attributes, I was thinking of potentially adding an inputFormatter prop to <TextField> (similar to dateFormatter in <DateField>), which would be a custom function run before the onBlur and onChange callback.

This obviously sounds similar to the masking issue, and I think theres a good amount of overlap between the two use cases of a mask (for formatting/adding symbols) and for those native attributes (for validation/specifying allowed user input). Maybe we could pick a solution that accomplishes both? or maybe the two cases need to be more clearly defined and separated? @line47 @pwolfert thoughts?

pwolfert commented 4 years ago

@bernardwang, I just tested out a TextField that had a pattern attribute inside a form, and HTML5 validation worked. I also tried maxlength, and that worked. I like the idea of a single interface for handling restrictions or formatting, though, because we can make sure they're done in an accessible way. Could you give an example of what you're thinking?

bernardwang commented 4 years ago

Right they still work with HTML5 validation, I was just assuming most React projects wouldn't use HTML5 validation. So we should continue to allow HTML5 validation, but maybe provide a recommended "Reacty" way of doing it too.

I also don't have evidence to back up the assumption that React projects arent using HTML5 validation, I'm just guessing they won't since our form components provide required and errorMessage props for validation. Its probably worth researching how others are validating their forms

pwolfert commented 4 years ago

👍 Maybe we can also research if there's any accessibility concerns with using HTML5 validation

bernardwang commented 4 years ago

Oh and the other issue with HTML5 is our form components including ChoiceList, TextField, DateField are abstractions above the actual HTML elements, so it's a bit of pain to use. I.e., min/max has to be with a type=number and maxlength/minlength is for type=text.

@dcloud explains some of this much better here: https://github.com/CMSgov/design-system/issues/390#issuecomment-521253797

bernardwang commented 4 years ago

And as far as an example, I was thinking something like this:

 const positiveCurrency = (currencyString) => {
   // This formatting occurs after the mask is applied, and returned via the `onChange` and `onBlur callbacks
   // This is all string manipulation which is both annoying and a bit fragile. Could use regex instead? 
   // Not sure what's the best solution here
   let formattedCurrency = currencyString
   if (formattedCurrency.chatAt(0) === '-') {
        formattedCurrency = formattedCurrency.substr(1);
   }
   if (formattedCurrency.length > '35') {
       // Can be handled whatever way they need
   }
   return formattedCurrency
};

<TextField 
   mask="currency"
   inputFormatter={positiveCurrency}
   onChange={(maskedCurrency) => this.setState({'currency', unmask(maskedCurrency)})}
/>
bernardwang commented 4 years ago

Although honestly, looking at that example, it's really not that helpful to add the inputFormatter because the user can do the same thing really easily. So maybe your original idea for a custom mask prop that allows for reusing of predefined masks is better. It would just be nice to have that new functionality also be able to handle use cases like maxlength or min.

dcloud commented 4 years ago

My quick take

I don't think masks should be used to restrict or modify input. I think restricting input could be acceptable when paired with clear instructions about why input is expected. I don't think we should transparently alter user input (transforming a negative number to a positive one, for example). I think input constraints might be useful as long as we are careful to inform the people of what those constraints are.

My longer take

It sounds like we are talking about a few things:

  1. how an input "provides visual and non-visual cues to a person about the expected value"
  2. how the input field might manage the "unmasked" value (vs the masked version).
  3. how (and when) we might restrict a person from entering invalid values (input constraints)
  4. validation when a person has input something (or finished entering information in all fields)

Masked fields solve 1 and 2, but don't support a way to create new masks. I don't think Masked fields attempt to address 3, or at least I can go into https://design.cms.gov/example/components.masked-field.react/ and enter non-numeric info in all of the masked fields, though they all represent numeric inputs. From what I've seen, validation (4), is assumed to be the application's responsibility.

I think 1&2, the "Mask" part of Masked fields, are about helping people see if what they typed in appears to be correct. Hyphens visually break phone numbers and SSNs into smaller pieces, helping people to see if what they typed looks correct. I think one reason we typically don't validate while a person is typing is because it can be jarring/distracting to see error messages while typing. It seems like the preferred method is to validate after a person has finished a form, and let them know where they need to make changes.

That said, I don't know if input constraints cause frustration for people (e.g.: "why can't I type letters in this field?"). It seems as if an input constraint may work when paired with a cue, such as via a mask. So if you need people to input positive numbers or integers, what sorts of cues would you provide that an input will not accept negative numbers or decimals?

Perhaps allowing for custom masks, per https://github.com/CMSgov/design-system/issues/514#issuecomment-541857376, might allow designers and developers to create custom masks and develop guidance on how to use input constraints to enhance that experience? I'd think we'd want to make sure we inform people about what is acceptable input before possibly constraining that input.

bernardwang commented 4 years ago

@dcloud Thanks for the breakdown, it clarifies the conversation a bit.

I wanted to clarify that Masks don't restrict characters from being entered, but in some cases they "constrain input" by removing non-numerics (altering user input) on blur. For example, this is the ssn mask behavior:

"badinput" - onBlur -> "badinput" // no change
"123bad456789input" - onBlur -> "123-45-6789" // invalid chars removed and masked applied

I don't know if this Mask behavior counts as 3, but I agree with you that it makes sense for Masks to only handle 1 & 2, as their main function is supposed to be helping readability. I also think "input constraints" sounds more suited for the positive currency use case, so preventing "-" from being entered, rather than a Mask which would remove the "-" sign on blur.

Earlier in this thread I thought that we could solve multiple things with custom masks, but now I'm thinking it might be more worth it to solidifying our guidance and definitions for Mask vs "input constraints", so that we can differentiate the two more.

pwolfert commented 2 years ago

Closing this two years later. Daniel had some really good points to make, and the new label-masked text field that we're making will take some of those points into account. My old thinking of having the component dealing with masking and unmasking and all that was unnecessarily complex and made for a confusing developer experience. I have high hopes that our new API will be much simpler to use, more flexible, and provide a better experience for all users.