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

🐛 Bug Report - Typing + into input does not display it until follow up digit is entered #54

Open
arzatskis opened this issue Sep 17, 2024 · 9 comments
Labels
bug Something isn't working

Comments

@arzatskis
Copy link

Bug description

When starting to type international number in empty input, entering + doesn't display it right away, only when entering number after that. It seems confusing and makes end users wonder if something is not working.

I'd expect seeing + in input right away.

Didn't dig too much into the code, but seems that problem is with getFormattedNumber(), which returns empty string if passed raw value is +. It's used internally in useMask's onInput handler and event.target.value is mutated / cleared, as method return empty string in that case.

Maybe that's expected behaviour. If so, would like to understand motivation behind this

As workaround for this, I'm not calling onInput if input value is +

// ...

const { onInput, onKeyDown } = useMask(pattern);

return {
  <input 
    onKeyDown={onKeyDown}
    onInput={(e) => {
      if (e.target.value === '+') return;
      onInput(e);
    }}
   />
}

Reproduction URL

https://playground.typesnippet.org/mui-phone-input/

Reproduction steps

  • Clear pre-filled country code
  • Enter + with keyboard - no change in input

Screenshots

DESCRIPTION

Logs

No response

Browsers

Safari

OS

Mac

@arzatskis arzatskis added the bug Something isn't working label Sep 17, 2024
@ArtyomVancyan
Copy link
Member

I see what you mean, but I would not consider it a bug or even an issue. Users don't have to input +, but I agree that this might be an optional feature. @arzatskis, would you like to contribute to the project and implement this optional feature so that it doesn't break the behavior of its dependents?

@arzatskis
Copy link
Author

Sure, I can try if you point me to correct way :)

Should this be handled in getFormattedNumber with some kind of option for hook? usePhone({ allowEnterringInternationalPrefix: true })? (didn't think much about the name, suggestions welcome)

@ArtyomVancyan
Copy link
Member

I think this must have been an option for the useMask hook. So useMask(pattern, options) where options is an optional argument. Also, you can just make the + prefix unremovable and fix the problem without additional chemistry that can break anything. You can start the react-phone-hooks/development application to test your changes runtime.

@arzatskis
Copy link
Author

arzatskis commented Sep 17, 2024

Ok, I'll take a stab at this this week hopefully - finishing implementation of phone input using the lib at the moment at my company, will know more by then.

I was following examples in /development dir and getFormattedNumber is used in other effects too, e.g.:

const formattedNumber = getFormattedNumber(event.target.value, pattern);

const formattedNumber = getFormattedNumber(initialValue, pattern);

This also clears entered + even if I add condition in onInput={} handler mentioned above. That's why I think we would need to have option for this method.

To overcome this, i made a wrapper. Seems to work..

import { getFormattedNumber as getFormattedNumberCore } from 'react-phone-hooks`; 

export const getFormattedNumber = (raw: string, pattern: string) => {
  // do not clear entered plus sign
  if (raw === '+') return raw;
  return getFormattedNumberCore(raw, pattern);
}

@ArtyomVancyan
Copy link
Member

Okay, and what about having a static + prefix that cannot be removed?

@arzatskis
Copy link
Author

arzatskis commented Sep 17, 2024

One downside I see with + always in input is that users won't be able to use placeholder on <input /> elements, as it will always have value entered. People may also may want to use CSS :placeholder-shown pseudo selector to detect empty input, which won't work etc.

It will probably lead to more overhead / cases to handle - target.value will always be there and more work will be needed to check if input is empty / nothing was entered by the user. One example that pops to mind: forms inputs may be validated on blur, but only if user entered something to input / element. If + is there by default, it will be treated as if value is there, even if user did not type anything into it.

Another problem may be with form libraries (e.g. Formik) which uses initialValues object (e.g. initialValues: { phone: '' }), yet input's value gonna be set to +, which may not be unintuitive / get in the way.

This may also can get in the way if in the future there's gonna be a feature to enter phones in national format (without + / country code).

Just speculating of course, but somehow strongly leaning towards option for hooks, where + is added / kept only when user intentionally types digits / sign by themselves.

@ArtyomVancyan
Copy link
Member

Okay, I see your point, but what about not including it in the value? It can be a static part of the input component outside the input box.

Interesting point of view, by the way :) Like the way you think.

@arzatskis
Copy link
Author

but what about not including it in the value

This will break current behaviour / we'll need changing formatted value displayed in input and won't fix original issue: typing + won't do anything. Additional you'll need to handle cases when someone pastes in phone number with + to remove it etc. (otherwise theres + before input and one in input...). Simillar issues if you want to copy value form input - won't be able to select + outside.

Another big issue is that this will require more work with styles. I assume how this is used is something like:

<div className="flex items-center gap-4">
  <select />
  <input />
</div>

// ...or (most likely), from existing components in your design system

<FormRow>
  <SelectField />
  <Text>+</Text>
  <TextField />
</FormFow>

// ...or 

<TextField leftElement={<SelectField />} /> 
  1. You'll need additional wrapper for <div>+ <input /></div>, so there's no spacing in case <FormRow /> has default gap set
  2. You'll need additional styles to remove left padding on <input />

Beauty and selling point of this lib is that it doesn't do too much and just provides hook / props for your inputs and that's it. Then developers can reuse their own components without thinking too much / needing to worry about setting this up properly. Suggest not throwing that away :)

@ArtyomVancyan
Copy link
Member

Okay, let's proceed with the initial scenario.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants