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

Allow passing custom regex objects as props in order to handle more complex format strings #2

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Osato28
Copy link

@Osato28 Osato28 commented Aug 18, 2022

Reason for changes

Because with an international phone format like +1 (###) ##-##-##, the current regex for stripping is insufficient:

  1. It allows letters, not just numbers;
  2. If you test such a format with the version currently on the main branch, you'll see that there's an issue where every time onChange runs on the component, it duplicates all allowed characters in the format string (in this case, 1).

An obvious fix would be to:

  1. strip +1 from the beginning of the input's value,
  2. and to allow only numbers.

In order to do both of those, custom regex objects need to be passed into the Input component.

I already fixed the problem while making a fork of this repo work in my own project, so instead of opening an issue I'm submitting fixed code as a pull request.

What has been done

  1. Added strip: RegExp and allow: RegExp props into the Input component.
    Reason: The former is used to find all pattern characters (the ones to be stripped), the latter is used to determine all allowed characters.
    This should not break backwards compatibility, since I have added old regex values as default values for those props.
  2. Made stripPatternCharacters and isUserCharacter functions accept regex as their second parameter.
    Reason: so that Input can pass custom strip and allow props into those functions.
  3. Renamed isUserCharacter to matchesRegex.
    Reason: otherwise changes on line 95-97 (making patternCharacterDeleted check for all characters that must be stripped) result in counterintuitive code (since isUserCharacter would be called to check with stripRegex, not with allowRegex).
  4. Made sure the cursor would be moved to the first placeholder character if it's located before the first placeholder character.
    Introduced an updatedCursorPosition constant (line 85 onwards) to keep track of the updated cursor position.
    Reason: otherwise the order in which characters are inserted breaks with complicated formats such as +1 (###) ##-##-##.
  5. Moved placeholder constant from function format() into the global scope.
    Reason: accessing it from inside the Input component is part of the change 2.
  6. Commented those parts of the code that have been changed.
    To make my changes easier to track, I have included comments that start with // CHANGE:.
  7. Updated "jest-dom@4.2.0" to "@testing-library/jest-dom@5.16.5". Reason: because "jest-dom" is deprecated (according to npm, at least).
  8. Updated docs/index.md to explain and demonstrate the use-case for new props.
  9. Added two test cases to the end of index.spec.js.

Unfortunately, all of those changes are intertwined (changes 2-5 are necessary to make change 1 work properly), so I couldn't separate them into different pull requests.

Known issues with this pull request

  1. For some reason, testing with jest doesn't act exactly like testing in browser.
    How to replicate: in the last test in index.spec.js, you will see that changing line 89 to input.setSelectionRange(0, 0); makes jest input characters at the end of the string.
    I tested this manually, and this issue does not replicate in a live browser.
  2. If, with a format such as +44 (###) ##-##-##, you move the cursor to the position such as (1, 1) or (2, 2) (after the + and before the last 4), every onChange still duplicates the numbers.
    I'm not sure why this happens: rubber-duck-debugging the code on lines 124-142 didn't produce any obvious answers.

Need feedback:

  • On whether submitting a pull request without opening an issue first is acceptable. This is my first pull request, so I'm not quite clear on this.
  • On the new documentation file. The new version is currently in the docs/index.md file; I did not change README.md.
  • On known issue 2. I put in some extra work to make sure it happens extremely rarely (doesn't happen if you put the cursor at the very beginning of the input), but it still happens and I haven't figured out why.

@danielyefet
Copy link
Owner

Thank you for this - it looks like a very valuable addition! I'm pretty swamped for the next week but I'll try to find some time to review it and we'll get it merged ASAP 👍

@danielyefet
Copy link
Owner

Hi @Osato28! I think your implementation is excellent and certainly solves the problem . My only concern is the developer experience while using the component - I want to keep it as simple as possible. I created this component because I couldn't find anything else on npm that didn't require a ton of config, which I wanted to avoid. Do you think you might be able to fix the issue whilst still keeping a single format prop? I'll have a look too - there must be a way...

@Osato28
Copy link
Author

Osato28 commented Sep 3, 2022

I did consider automatically generating stripRegex from the format string. The basic algorithm is to:

  1. splice the format string into an array of substrings that lie between placeholder characters
  2. check if allowRegex.test yields true on any of the substrings,
  3. parse those substrings to add \ before each character that is reserved in regex
  4. add parsed substrings to stripRegexString as (${parsedSubstring})
  5. after each substring, add |
  6. add [^${allowRegex}] to the end of stripRegexString
  7. and turn stripRegexString into a regex object with stripRegex = new RegExp(stripRegexString, 'g').

However, at this stage I've decided against it because:

  1. I hadn't figured out how to fix known issue 2 with this PR. In fact, I haven't even found its cause.
    And until all known issues with my solution are resolved, I cannot be sure the regex strings I used are so correct that it's safe to generate them by default.

  2. The form's default behavior was unfit for a telephone input because it allowed the user to enter letters instead of numbers.
    So this component still needs customizable regex fields for specific use-cases. I would be able to remove stripRegex from props, but not allowRegex.

  3. I didn't have time to test every common format (in fact, many formats such as emails would require rewriting the component's core logic to add a wildcard placeholder for an unknown amount of characters), so I couldn't be sure my algorithm was universally applicable. Other issues might pop up with formats that I haven't tested.


UPD: I could add automatic generation of stripRegex if you want, but I'm not sure when I'd be free enough to do so. I'm a bit swamped in work myself, unfortunately.

@danielyefet
Copy link
Owner

That's a great idea about generating the strip regex - I can see that working well. I get what you mean about the known issue, though - it makes sense to make it work 100% first. I know what you mean about the need to restrict characters, too, e.g. with a phone number.

Don't even stress about not having time - it's hard to do this stuff alongside a day job!

Let's leave this open, and I'll work on it as soon as I get some time. I'm keen to make it work for this use case, but I want to ponder the solution a bit more, if you don't mind!

Appreciate your help so far 🙂

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.

2 participants