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

Fix decimalSeparator with a suffix containing more than one character #743

Closed

Conversation

sormpe
Copy link

@sormpe sormpe commented Mar 8, 2023

Describe the issue/change

This PR fixes issue described in #725

Describe the changes proposed/implemented in this PR

As romanopassalacqua suggested, we now check if char is in allowedDecimalSeparators in getCaretPosition. Before we only checked if chars were the same resulting that indexMap was not affected. This caused problem described in #725 when using some of the allowedDecimalSeparators which is not (default) decimalSeparator.

Link Github issue if this PR solved an existing issue

#725

Example usage (If applicable)

        <div className="example">
          <h3>A suffix containing more than one character</h3>
          <NumericFormat
            suffix={' €'}
            allowedDecimalSeparators={['.', ',']}
          />
        </div>

        <div className="example">
          <h3>A long suffix</h3>
          <NumericFormat
            suffix={'longsuffix'}
            allowedDecimalSeparators={['.', ',']}
          />
        </div>

        <div className="example">
          <h3>A superlong suffix</h3>
          <NumericFormat
            suffix={'superlongsuffix with spaces'}
            allowedDecimalSeparators={['.', ',']}
          />
        </div>

Video

react-number-format-fix.mp4

Please check which browsers were used for testing

  • Chrome
  • Chrome (Android)
  • Safari (OSX)
  • Safari (iOS)
  • Firefox
  • Firefox (Android)

@sormpe
Copy link
Author

sormpe commented Mar 8, 2023

Apparently this change break some tests, could you @s-yadav check this out?

EDIT: made a small fix, now all tests pass

@sormpe
Copy link
Author

sormpe commented Mar 9, 2023

I noticed that my fix works only if decimal separator is the first char we type into. In other cases startIndex should be one greater. We can check if allowed decimal separator was used by adding boolean flag, something like this:

export function getCaretPosition(
  newFormattedValue: string,
  lastFormattedValue: string,
  curValue: string,
  curCaretPos: number,
  boundary: boolean[],
  isValidInputCharacter: (char: string) => boolean,
) {
  /**
   * if something got inserted on empty value, add the formatted character before the current value,
   * This is to avoid the case where typed character is present on format characters
   */

  const firstAllowedPosition = boundary.findIndex((b) => b);
  const prefixFormat = newFormattedValue.slice(0, firstAllowedPosition);
  if (!lastFormattedValue && !curValue.startsWith(prefixFormat)) {
    curValue = prefixFormat + curValue;
    curCaretPos = curCaretPos + prefixFormat.length;
  }

  const curValLn = curValue.length;
  const formattedValueLn = newFormattedValue.length;

  // create index map
  const addedIndexMap: { [key: number]: boolean } = {};
  const indexMap = new Array(curValLn);

  let wasAllowedDecimalSeparatorUsed = false;

  for (let i = 0; i < curValLn; i++) {
    indexMap[i] = -1;
    for (let j = 0, jLn = formattedValueLn; j < jLn; j++) {
      const isAllowedDecimalSeparator =
        isValidInputCharacter(curValue[i]) && isValidInputCharacter(newFormattedValue[j]);
      if (
        (curValue[i] === newFormattedValue[j] && addedIndexMap[j] !== true) ||
        (isAllowedDecimalSeparator && i === 0 && j === 0)
      ) {
        indexMap[i] = j;
        addedIndexMap[j] = true;
        // ADD FLAG HERE!
        wasAllowedDecimalSeparatorUsed = true;
        break;
      }
    }
  }

  /**
   * For current caret position find closest characters (left and right side)
   * which are properly mapped to formatted value.
   * The idea is that the new caret position will exist always in the boundary of
   * that mapped index
   */
  let pos = curCaretPos;
  while (pos < curValLn && (indexMap[pos] === -1 || !isValidInputCharacter(curValue[pos]))) {
    pos++;
  }

  // if the caret position is on last keep the endIndex as last for formatted value
  const endIndex = pos === curValLn || indexMap[pos] === -1 ? formattedValueLn : indexMap[pos];

  pos = curCaretPos - 1;
  while (pos > 0 && indexMap[pos] === -1) pos--;
  const startIndex = pos === -1 || indexMap[pos] === -1 ? 0 : indexMap[pos] + 1;

  /**
   * case where a char is added on suffix and removed from middle, example 2sq345 becoming $2,345 sq
   * there is still a mapping but the order of start index and end index is changed
   */
  if (startIndex > endIndex) return endIndex;

  /**
   * given the current caret position if it closer to startIndex
   * keep the new caret position on start index or keep it closer to endIndex
   */

// USE FLAG HERE!
 if (wasAllowedDecimalSeparatorUsed) {
    return curCaretPos - startIndex < endIndex - curCaretPos ? startIndex + 1 : endIndex;
  }

  return curCaretPos - startIndex < endIndex - curCaretPos ? startIndex : endIndex;
}

This, however, break the tests, and I'm unsure if it break functionality in some other cases.

@s-yadav
Copy link
Owner

s-yadav commented Apr 30, 2023

Hey @sormpe Thanks for PR. couldn't look into this earlier, will check this.

@s-yadav
Copy link
Owner

s-yadav commented Aug 29, 2023

As this was incorporated with some other fix. closing the PR.

@s-yadav s-yadav closed this Aug 29, 2023
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