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 1 commit
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
15 changes: 15 additions & 0 deletions 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 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.");
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
5 changes: 4 additions & 1 deletion 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 @@ -848,7 +848,10 @@ export class InputNumber
: sanitizedValue;

const newLocalizedValue = numberStringFormatter.localize(newValue);
this.localizedValue = newLocalizedValue;
this.localizedValue =
value.charAt(value.length - 1) === numberStringFormatter.decimal
? `${newLocalizedValue}${numberStringFormatter.decimal}`
: newLocalizedValue;

this.setPreviousNumberValue(previousValue ?? this.value);
this.previousValueOrigin = origin;
Expand Down
15 changes: 15 additions & 0 deletions 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
5 changes: 4 additions & 1 deletion 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 @@ -984,7 +984,10 @@ export class Input
: sanitizedValue;

const newLocalizedValue = numberStringFormatter.localize(newValue);
this.localizedValue = newLocalizedValue;
this.localizedValue =
value.charAt(value.length - 1) === numberStringFormatter.decimal
? `${newLocalizedValue}${numberStringFormatter.decimal}`
: newLocalizedValue;

this.userChangedValue = origin === "user" && this.value !== newValue;
// don't sanitize the start of negative/decimal numbers, but
Expand Down