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.

Sign up for GitHub

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(input, input-number): no longer removes trailing decimal separator #7159

Merged
merged 29 commits into from
Jun 29, 2023
Merged

fix(input, input-number): no longer removes trailing decimal separator #7159

Show file tree
Hide file tree
Changes from 13 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
59b5a85
fix(input): no longer remove trailing decimal separator
anveshmekala Jun 8, 2023
4613212
add localized leading decimal zero values
anveshmekala Jun 16, 2023
f241c17
Merge branch 'master' into anveshmekala/7039-fix-calcite-input-decima…
anveshmekala Jun 16, 2023
387db5b
Merge branch 'master' into anveshmekala/7039-fix-calcite-input-decima…
anveshmekala Jun 16, 2023
54b7143
Merge branch 'master' into anveshmekala/7039-fix-calcite-input-decima…
anveshmekala Jun 20, 2023
5d6f507
fix tests
anveshmekala Jun 20, 2023
c6a1e71
fix tests
anveshmekala Jun 21, 2023
cffe278
Merge branch 'master' into anveshmekala/7039-fix-calcite-input-decima…
anveshmekala Jun 21, 2023
1c60d07
cleanup
anveshmekala Jun 21, 2023
eacde2d
Merge branch 'master' into anveshmekala/7039-fix-calcite-input-decima…
anveshmekala Jun 21, 2023
c447239
refactor
anveshmekala Jun 21, 2023
7a0480a
refactor util method
anveshmekala Jun 21, 2023
87f15d7
remove redundant code blocks
anveshmekala Jun 21, 2023
63dff04
refactor util method to handle comma decimal separator
anveshmekala Jun 23, 2023
d06003b
Merge branch 'master' into anveshmekala/7039-fix-calcite-input-decima…
anveshmekala Jun 26, 2023
6751aaa
refactor util methods
anveshmekala Jun 26, 2023
688b98a
update tests
anveshmekala Jun 26, 2023
f7b9f94
remove commented code
anveshmekala Jun 26, 2023
43b3075
add missing type for tests
anveshmekala Jun 26, 2023
cd4a3ac
fix failing spec tests
anveshmekala Jun 27, 2023
98c86d4
use regex
anveshmekala Jun 27, 2023
ce04bab
add more spec tests
anveshmekala Jun 27, 2023
ecbc697
remove unused imports
anveshmekala Jun 27, 2023
fbc3831
Merge branch 'master' into anveshmekala/7039-fix-calcite-input-decima…
anveshmekala Jun 27, 2023
de31f7c
simplify util method logic
anveshmekala Jun 28, 2023
05b771c
Merge branch 'master' into anveshmekala/7039-fix-calcite-input-decima…
anveshmekala Jun 28, 2023
c507400
feedback changes and more e2e tests added
anveshmekala Jun 29, 2023
4a57130
remove unused methods in number spec test
anveshmekala Jun 29, 2023
dfc9311
add new lines in test
anveshmekala Jun 29, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
74 changes: 73 additions & 1 deletion packages/calcite-components/src/components/input-number/input-number.e2e.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -715,6 +715,21 @@ describe("calcite-input-number", () => {
const input = await page.find("calcite-input-number >>> input");
expect(await input.getProperty("value")).toBe("2");
});

it("allows trailing decimal separator", async () => {
const page = await newE2EPage();
await page.setContent(html`<calcite-input-number value="1.1"></calcite-input-number>`);
const calciteInput = await page.find("calcite-input-number");
const inputEventSpy = await calciteInput.spyOnEvent("calciteInputNumberInput");

await calciteInput.click();
await page.keyboard.press("Backspace");
await page.waitForChanges();
expect(inputEventSpy).toHaveReceivedEvent();
const input = await page.find("calcite-input-number >>> input");

expect(await input.getProperty("value")).toBe("1.");
Copy link
Member

Choose a reason for hiding this comment

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

the component's value should still be 1. You will need to go into the shadowRoot and check the value of the internal input to verify it is 1.

Copy link
Member

Choose a reason for hiding this comment

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

Nevermind that is what you were doing it was other input tests failing. I need read closer before opening my big mouth lol

Copy link
Contributor Author

@anveshmekala anveshmekala Jun 9, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

no problem. The issues with other tests is the immutation of localizedValue when the user tries to delete the decimal part. The logic applies even if the user is typing out a value like 0.05. By the time user types 0.0 the validation is happening hence changing the localizedValue to 0 again. Current logic in default branch avoids this by not changing the localizedValue variable until the user inputs a valid value which allows users to type the value of their without any auto correction.

Trying to figure out a reliable way to identify if the user is adding a value in the input field or deleting the value so that we can apply the logic only for deletion case.

Copy link
Member

Choose a reason for hiding this comment

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

Gotcha.

Trying to figure out a reliable way to identify if the user is adding a value in the input field or deleting the value so that we can apply the logic only for deletion case.

Some potential options for investigation:

  1. only sanitzing the trailing decimal if the length of the previous value is more than the current one
  2. only sanitzing the trailing decimal on backspace and delete key events. And the increment down click event too probably

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah i was using the second option which is not feasible with inputEvent . i had to create a combination of above. Realized there was an another bug with the input when the user has leading 0's in the decimal. If the user deletes the 2 in 0.00002 then the value becomes 0 which is similar to #7039 . I tried adding those trailing zero's back in the decimal but the localize method is removing them too. We may have to follow similar technique of adding back those zeros like we did for decimal separator.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

or we can refactor this method.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah the input lifecycle methods will probably need a refactor. Once you start doing crazy stuff like this to fix bugs you know you've gone too far lol.

});
});

describe("emits events when value is modified", () => {
Expand Down Expand Up @@ -1360,7 +1375,7 @@ describe("calcite-input-number", () => {

await page.keyboard.press("Backspace");
await page.waitForChanges();
expect(await element.getProperty("value")).toBe("1");
expect(await element.getProperty("value")).toBe("1.");
expect(calciteInputNumberInput).toHaveReceivedEventTimes(1);
});

Expand All @@ -1385,6 +1400,63 @@ describe("calcite-input-number", () => {
expect(await element.getProperty("value")).toBe("10000000");
});

it("should be able to append value to zeros in decimal after Backspace", async () => {
const page = await newE2EPage();
await page.setContent(`
<calcite-input-number></calcite-input-number>
`);

const element = await page.find("calcite-input-number");
await element.callMethod("setFocus");
await typeNumberValue(page, "0.0000");
await page.waitForChanges();
expect(await element.getProperty("value")).toBe("0.0000");

await page.keyboard.press("Backspace");
await typeNumberValue(page, "1");
await page.waitForChanges();
expect(await element.getProperty("value")).toBe("0.0001");

await typeNumberValue(page, "01");
await page.waitForChanges();
expect(await element.getProperty("value")).toBe("0.000101");
});

it("should keep leading decimal separator while input is focused on Backspace", async () => {
const page = await newE2EPage();
await page.setContent(`
<calcite-input-number></calcite-input-number>
`);

const element = await page.find("calcite-input-number");
await element.callMethod("setFocus");
await typeNumberValue(page, "0.01");
await page.waitForChanges();
expect(await element.getProperty("value")).toBe("0.01");

await page.keyboard.press("Backspace");
await page.waitForChanges();
expect(await element.getProperty("value")).toBe("0.0");

await page.keyboard.press("Backspace");
await page.waitForChanges();
expect(await element.getProperty("value")).toBe("0.");

await typeNumberValue(page, "01");
await page.waitForChanges();
expect(await element.getProperty("value")).toBe("0.01");
});

it("should sanitize leading decimal zeros on initial render", async () => {
const page = await newE2EPage();
await page.setContent(html`<calcite-input-number value="0.0000"></calcite-input-number>`);
const calciteInput = await page.find("calcite-input-number");
const input = await page.find("calcite-input-number >>> input");

expect(await input.getProperty("value")).toBe("0");
expect(await calciteInput.getProperty("value")).toBe("0.0000");
});

it("sanitize extra dashes from value", async () => {
const page = await newE2EPage();
await page.setContent(`<calcite-input-number></calcite-input-number>`);
Expand Down
45 changes: 37 additions & 8 deletions packages/calcite-components/src/components/input-number/input-number.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ import {
numberStringFormatter
} from "../../utils/locale";
import {
addLocalizedTrailingDecimalZeros,
BigDecimal,
isValidNumber,
parseNumberString,
Expand Down Expand Up @@ -840,12 +841,22 @@ export class InputNumber
useGrouping: this.groupSeparator
};

const sanitizedValue = sanitizeNumberString(
// no need to delocalize a string that ia already in latn numerals
(this.numberingSystem && this.numberingSystem !== "latn") || defaultNumberingSystem !== "latn"
? numberStringFormatter.delocalize(value)
: value
);
const isValueDeleted =
this.previousValue?.length > value.length || this.value?.length > value.length;

const hasTrailingDecimalSeparator =
value.charAt(value.length - 1) === numberStringFormatter.decimal;

const sanitizedValue =
hasTrailingDecimalSeparator && isValueDeleted
? value
Copy link
Member

Choose a reason for hiding this comment

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

Is there a reason why the value doesn't need to be delocalized when there is a trailing 0 and it is a delete action?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i assume you are referring to trailingDecimalSeparator , the value is already delocalized by then.

Copy link
Member

Choose a reason for hiding this comment

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

If the value is already delocalized by then, we could remove the delocalization a couple lines down right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i think so, it was added here . When removed , all the tests are passing which is a good sign.

Copy link
Member

Choose a reason for hiding this comment

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

Are we sure it is already delocalized in all cases? (e.g. typing, pasting, and setting the value via JS?)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yup, while typing or pasting InputNumberInput handler will delocalize the strings. For setting the value via JS, i don't think users can parse non latin number values which sounds strange tbh. I guess it is intentional to avoid localization twice.

: sanitizeNumberString(
// no need to delocalize a string that ia already in latn numerals
(this.numberingSystem && this.numberingSystem !== "latn") ||
defaultNumberingSystem !== "latn"
? numberStringFormatter.delocalize(value)
: value
);

const newValue =
value && !sanitizedValue
Expand All @@ -854,8 +865,26 @@ export class InputNumber
: ""
: sanitizedValue;

const newLocalizedValue = numberStringFormatter.localize(newValue);
this.localizedValue = newLocalizedValue;
let newLocalizedValue = numberStringFormatter.localize(newValue);

// adds localized trailing decimal zero values
if (
newLocalizedValue.length !== newValue.length &&
origin !== "connected" &&
!hasTrailingDecimalSeparator
) {
newLocalizedValue = addLocalizedTrailingDecimalZeros(
newLocalizedValue,
newValue,
numberStringFormatter
);
}

// adds localized trailing decimal separator
this.localizedValue =
hasTrailingDecimalSeparator && isValueDeleted
? `${newLocalizedValue}${numberStringFormatter.decimal}`
: newLocalizedValue;

this.setPreviousNumberValue(previousValue ?? this.value);
this.previousValueOrigin = origin;
Expand Down
74 changes: 73 additions & 1 deletion packages/calcite-components/src/components/input/input.e2e.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -701,6 +701,21 @@ describe("calcite-input", () => {
expect(inputEventSpy).not.toHaveReceivedEvent();
expect(changeEventSpy).not.toHaveReceivedEvent();
});

it("allows trailing decimal separator", async () => {
const page = await newE2EPage();
await page.setContent(html`<calcite-input value="1.1"></calcite-input>`);
const calciteInput = await page.find("calcite-input");
const inputEventSpy = await calciteInput.spyOnEvent("calciteInputInput");

await calciteInput.click();
await page.keyboard.press("Backspace");
await page.waitForChanges();
expect(inputEventSpy).toHaveReceivedEvent();
const input = await page.find("calcite-input >>> input");

expect(await input.getProperty("value")).toBe("1.");
});
});

describe("emits events when value is modified", () => {
Expand Down Expand Up @@ -1538,7 +1553,7 @@ describe("calcite-input", () => {

await page.keyboard.press("Backspace");
await page.waitForChanges();
expect(await element.getProperty("value")).toBe("1");
expect(await element.getProperty("value")).toBe("1.");
expect(calciteInputInput).toHaveReceivedEventTimes(1);
});

Expand All @@ -1563,6 +1578,63 @@ describe("calcite-input", () => {
expect(await element.getProperty("value")).toBe("10000000");
});

it("should be able to append value to zeros in decimal after Backspace", async () => {
const page = await newE2EPage();
await page.setContent(`
<calcite-input-number></calcite-input-number>
`);

const element = await page.find("calcite-input-number");
await element.callMethod("setFocus");
await typeNumberValue(page, "0.0000");
await page.waitForChanges();
expect(await element.getProperty("value")).toBe("0.0000");

await page.keyboard.press("Backspace");
await typeNumberValue(page, "1");
await page.waitForChanges();
expect(await element.getProperty("value")).toBe("0.0001");

await typeNumberValue(page, "01");
await page.waitForChanges();
expect(await element.getProperty("value")).toBe("0.000101");
});

it("should keep leading decimal separator while input is focused on Backspace", async () => {
const page = await newE2EPage();
await page.setContent(`
<calcite-input-number></calcite-input-number>
`);

const element = await page.find("calcite-input-number");
await element.callMethod("setFocus");
await typeNumberValue(page, "0.01");
await page.waitForChanges();
expect(await element.getProperty("value")).toBe("0.01");

await page.keyboard.press("Backspace");
await page.waitForChanges();
expect(await element.getProperty("value")).toBe("0.0");

await page.keyboard.press("Backspace");
await page.waitForChanges();
expect(await element.getProperty("value")).toBe("0.");

await typeNumberValue(page, "01");
await page.waitForChanges();
expect(await element.getProperty("value")).toBe("0.01");
});

it("should sanitize leading decimal zeros on initial render", async () => {
const page = await newE2EPage();
await page.setContent(html`<calcite-input value="0.0000" type="number"></calcite-input>`);
const calciteInput = await page.find("calcite-input");
const input = await page.find("calcite-input >>> input");

expect(await input.getProperty("value")).toBe("0");
expect(await calciteInput.getProperty("value")).toBe("0.0000");
});

it("sanitize extra dashes from number input value", async () => {
const page = await newE2EPage();
await page.setContent(`<calcite-input type="number"></calcite-input>`);
Expand Down
46 changes: 37 additions & 9 deletions packages/calcite-components/src/components/input/input.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ import {
} from "../../utils/locale";

import {
addLocalizedTrailingDecimalZeros,
BigDecimal,
isValidNumber,
parseNumberString,
Expand Down Expand Up @@ -975,13 +976,22 @@ export class Input
signDisplay: "never"
};

const sanitizedValue = sanitizeNumberString(
// no need to delocalize a string that ia already in latn numerals
(this.numberingSystem && this.numberingSystem !== "latn") ||
defaultNumberingSystem !== "latn"
? numberStringFormatter.delocalize(value)
: value
);
const isValueDeleted =
this.previousValue?.length > value.length || this.value?.length > value.length;

const hasTrailingDecimalSeparator =
value.charAt(value.length - 1) === numberStringFormatter.decimal;

const sanitizedValue =
hasTrailingDecimalSeparator && isValueDeleted
? value
: sanitizeNumberString(
// no need to delocalize a string that ia already in latn numerals
(this.numberingSystem && this.numberingSystem !== "latn") ||
defaultNumberingSystem !== "latn"
? numberStringFormatter.delocalize(value)
: value
);

const newValue =
value && !sanitizedValue
Expand All @@ -990,8 +1000,26 @@ export class Input
: ""
: sanitizedValue;

const newLocalizedValue = numberStringFormatter.localize(newValue);
this.localizedValue = newLocalizedValue;
let newLocalizedValue = numberStringFormatter.localize(newValue);

// adds localized trailing decimal zero values
if (
newLocalizedValue.length !== newValue.length &&
origin !== "connected" &&
!hasTrailingDecimalSeparator
) {
newLocalizedValue = addLocalizedTrailingDecimalZeros(
newLocalizedValue,
newValue,
numberStringFormatter
);
}

// adds localized trailing decimal separator
this.localizedValue =
hasTrailingDecimalSeparator && isValueDeleted
? `${newLocalizedValue}${numberStringFormatter.decimal}`
: newLocalizedValue;

this.userChangedValue = origin === "user" && this.value !== newValue;
// don't sanitize the start of negative/decimal numbers, but
Expand Down
47 changes: 45 additions & 2 deletions packages/calcite-components/src/utils/number.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,14 +137,27 @@ export const sanitizeNumberString = (numberString: string): string =>
.replace(allHyphensExceptTheStart, "")
.replace(decimalOnlyAtEndOfString, "")
.replace(allLeadingZerosOptionallyNegative, "$1");

return isValidNumber(sanitizedValue)
? isNegativeDecimalOnlyZeros.test(sanitizedValue)
? sanitizedValue
: new BigDecimal(sanitizedValue).toString()
: getBigDecimalAsString(sanitizedValue)
: nonExpoNumString;
});

export function getBigDecimalAsString(sanitizedValue: string): string {
const decimals = sanitizedValue.split(".")[1];
const newdecimals = new BigDecimal(sanitizedValue).toString().split(".")[1];
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick:

const bigDecimalValue = new BigDecimal(sanitizedValue).toString()
const newDecimals = bigDecimalValue.split(".")[1]

And then you can return bigDecimalValue on line 158 instead of calling new BigDecimal() again.


// adds back trailing decimal zeros
if (decimals && !newdecimals && BigInt(decimals) === BigInt(0) && decimals !== newdecimals) {
const value = new BigDecimal(sanitizedValue).toString() + ".";
Copy link
Member

Choose a reason for hiding this comment

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

const value = `${bigDecimalValue}.`

const newvalue = value.padEnd(value.length + decimals.length, "0");
return newvalue;
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: we can skip the extra memory allocation required by declaring newvalue and just do:

return value.padEnd(value.length + decimals.length, "0");

Copy link
Member

Choose a reason for hiding this comment

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

I've been more conscious about overworking our garbage collectors after seeing the results of their strike in France.

}

return new BigDecimal(sanitizedValue).toString();
}

export function sanitizeExponentialNumberString(numberString: string, func: (s: string) => string): string {
if (!numberString) {
return numberString;
Expand Down Expand Up @@ -217,3 +230,33 @@ export function expandExponentialNumberString(numberString: string): string {
function stringContainsNumbers(string: string): boolean {
return numberKeys.some((number) => string.includes(number));
}

/**
* Adds localized trailing decimals zero values to the number string.
* BigInt conversion to string removes the trailing decimal zero values (Ex: 1.000 is returned as 1). This method helps adding them back.
*
* @param {string} localizedValue - localized number string value
* @param {string} value - current value in the input field
* @param {NumberStringFormat} numberStringFormatter - numberStringFormatter instance to localize the number value
* @returns {string} localized number string value
*/
export function addLocalizedTrailingDecimalZeros(
localizedValue: string,
value: string,
formatter: NumberStringFormat
): string {
let localizedDecimals;
const decimalSeparator = formatter.decimal;
Copy link
Member

Choose a reason for hiding this comment

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

Nitpick: this line can be moved into the if statement

if (localizedValue.includes(decimalSeparator)) {
localizedDecimals = localizedValue.split(".")[1];
}
const decimals = value.split(".")[1];

if (decimals && localizedDecimals !== decimals) {
localizedValue = localizedValue + decimalSeparator;
[...decimals].forEach((decimal) => {
localizedValue += formatter.localize(decimal);
});
}
return localizedValue;
}
Copy link
Member

Choose a reason for hiding this comment

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

This function is a bit confusing to me. Is it only for adding trailing 0s? If so, then why is it iterating over all of the initial value's decimal places and concat'ing them to the localizedValue? For example

value is 123.3210
localizedValue is 123,321

would this function return 123,321,3210?

Also, can you use template literals instead of concatenating and add some unit tests for this function.

Copy link
Member

Choose a reason for hiding this comment

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

Example of changing the concat'ing to template literals is this:

 if (decimals && localizedDecimals !== decimals) {
    localizedValue = localizedValue + decimalSeparator;
    [...decimals].forEach((decimal) => {
      localizedValue += formatter.localize(decimal);
    });
  }
  return localizedValue;

to something like:

return decimals && localizedDecimals !== decimals
    ? `${localizedValue}${decimalSeparator}${Array.from(decimals)
        .map((d) => formatter.localize(d))
        .join("")}`
    : localizedValue;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, it is supposed to add only trailing zeros since those are the ones that are trimmed off during localization and sanitization. Great catch and i agree that iteration can be avoided and didn't factored in numbering systems like french ☹️ . Will refactor this one and add a test as well.

Copy link
Member

@benelan benelan Jun 24, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

To clarify, the template literals vs string concatenation thing is about weirdness with RTL langauges. Other than that the two snippets above are pretty much the same. I just used map() instead of forEach() in that specific example.