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

anveshmekala · 2023-06-08T22:37:38Z

Related Issue: #7039

Summary

This PR will allow trailing decimal separator while user is editing the input value in calcite-input & calcite-input-number

Before the Change:

If the user removes the 1 value from 0.00001 , the input will be sanitized and the new value will be 0
If the user removes the 1 value from 0.1 , the input will be sanitized and the new value will be 0

After the Change:

If the user removes the 1 value from 0.01 , the input value will be 0.0
If the user removes the 1 value from 0.1 , the input value will be 0.

benelan · 2023-06-09T20:29:34Z

packages/calcite-components/src/components/input-number/input-number.e2e.ts

+      expect(inputEventSpy).toHaveReceivedEvent();
+      const input = await page.find("calcite-input >>> input");
+
+      expect(await input.getProperty("value")).toBe("1.");


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.

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

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.

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:

only sanitzing the trailing decimal if the length of the previous value is more than the current one

only sanitzing the trailing decimal on backspace and delete key events. And the increment down click event too probably

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.

or we can refactor this method.

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.

benelan

Try adding default decimal/group/minus values for en to NumberStringFormat.

https://github.com/esri/calcite-components/blob/dd1dfbdd2dc89ad5b2ea8be2f7adf70086217220/packages/calcite-components/src/utils/locale.ts#L322-L347

export class NumberStringFormat {
  /**
   * The actual group separator for the specified locale.
   * Some white space group separators don't render correctly in the browser,
   * so we replace them with a normal <SPACE>.
   */
  private _actualGroup = ",";

  /** the corrected group separator */
  private _group = ",";

  get group(): string {
    return this._group;
  }

  private _decimal = ".";

  get decimal(): string {
    return this._decimal;
  }

  private _minusSign = "-";

  get minusSign(): string {
    return this._minusSign;
  }

…l-separator

anveshmekala · 2023-06-21T22:48:55Z

Open for suggestions to avoid adding trailing decimal zero values during sanitization and also localization of the value.

benelan

A couple comments, but looking good nice work! I'll do some testing on my end too because changing the input's lifecycle methods can have some weird side effects.

I think eventually we will need a larger refactor because doing these case by case patches isn't sustainable.

benelan · 2023-06-22T18:37:33Z

packages/calcite-components/src/utils/number.ts

      : nonExpoNumString;
  });

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


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.

benelan · 2023-06-22T18:43:30Z

packages/calcite-components/src/utils/number.ts

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


const value = `${bigDecimalValue}.`

benelan · 2023-06-22T18:49:50Z

packages/calcite-components/src/utils/number.ts

+  if (decimals && !newdecimals && BigInt(decimals) === BigInt(0) && decimals !== newdecimals) {
+    const value = new BigDecimal(sanitizedValue).toString() + ".";
+    const newvalue = value.padEnd(value.length + decimals.length, "0");
+    return newvalue;


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

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

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

benelan · 2023-06-22T19:09:45Z

packages/calcite-components/src/utils/number.ts

+    });
+  }
+  return localizedValue;
+}


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.

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;

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.

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.

…l-separator

anveshmekala · 2023-06-28T19:26:22Z

Open question: Do we want to keep the trailing decimal zeros on initial render ?

Example:

<calcite-input-number value="123.0" ></calcite-input-number>

Would the above should display 123 or 123.0 in input field after this change

jcfranco

Code LGTM!

@benelan for another look-see. 1️⃣.0️⃣0️⃣0️⃣🔍👁️👄👁️

jcfranco · 2023-06-28T19:31:17Z

packages/calcite-components/src/utils/number.spec.ts

+      expect(addLocalizedTrailingDecimalZeros(localizedValue, "123456.000", numberStringFormatter)).toBe(result);
+    });
+
+    it(`returns same value if no trailing decimal zero value is removed- ${locale}`, () => {


Nitpick: ...removed- ${locale} ➡️ ...removed - ${locale} (space between -)

jcfranco · 2023-06-28T19:43:34Z

Open question: Do we want to keep the trailing decimal zeros on initial render ?

I think we'd want to keep them in order to align with developer expectations and with the behavior of adding it later if the value was initially 123.

anveshmekala · 2023-06-28T19:47:31Z

Open question: Do we want to keep the trailing decimal zeros on initial render ?

I think we'd want to keep them in order to align with developer expectations and with the behavior of adding it later if the value was initially 123.

makes sense, the current expectation is to keep it to 123.

benelan

Added a couple comments - there may be one more edge case concerning exponential numbers. After that LGTM, great stuff!

Any feedback on code clarity and/or places that could use some doc regarding the number sanitazation/localization logic would also be appreciated. I think you're the first person to touch that stuff since I did the big rework so I want to make sure its legible lol.

Do we want to keep the trailing decimal zeros on initial render ?

Great question. I don't have a strong preference either way, ~~@jcfranco @eriklharper any thoughts?~~ (nevermind, just saw Franco's thoughts)

FWIW the native input doesn't remove decimal or trailing zero so following their lead would make sense

benelan · 2023-06-28T23:02:13Z

packages/calcite-components/src/utils/number.spec.ts

+      const localizedZeroValue = numberStringFormatter.localize("0");
+      const result = `${localizedValue}${numberStringFormatter.decimal}`.padEnd(
+        localizedValue.length + 4,
+        localizedZeroValue


can you add a few more spec tests for the following numbers:

1230000000000000000000000000000.00000000000000000000045000000000000000000000000

-10.02100e10000

The regex may need to be adjusted to not consider trailing zeros after an e.

It would probably be easier to make sure indexOf("e") === -1 rather than adding a negative lookahead or something to the regex

Good catch, the 0 after the e should be ignored.

benelan · 2023-06-28T23:05:46Z

packages/calcite-components/src/components/input/input.e2e.ts

+          };
+          const input = await page.find("calcite-input >>> input");
+          expect(await input.getProperty("value")).toBe("0");
+        });


Can you add an e2e test (doesn't have to be for every locale) for this number and then type three backspaces: 2.100e10.

For context, 2.100e is NaN so I want to make sure the logic still works after deleting the e.

2.100e10 is auto-corrected to 2.1e1 by default.

benelan · 2023-06-28T23:16:56Z

packages/calcite-components/src/utils/number.ts

+  formatter: NumberStringFormat
+): string {
+  const trailingDecimalZeros = value.match(hasTrailingDecimalZeros)[0];
+  const decimalSeparator = formatter.decimal;


Nitpick: this line can be moved into the if statement

🤖 I have created a release *beep* *boop* --- <details><summary>@esri/calcite-components: 1.5.0</summary> ## [1.5.0](https://github.com/Esri/calcite-design-system/compare/@esri/[email protected]...@esri/[email protected]) (2023-08-03) ### Features * **action-group:** Adds overlayPositioning property. ([#7366](#7366)) ([ca9f35a](ca9f35a)) * Allow sharing focus trap stacks via configuration global ([#7334](#7334)) ([934a19f](934a19f)) * Automatically import and define Calcite Components when importing their React wrapper ([#7185](#7185)) ([bf0ff67](bf0ff67)) * **block, block-section:** Add setFocus method ([#7208](#7208)) ([35d4bbb](35d4bbb)) * **block:** Improve block's content layout to allow scrolling ([#7367](#7367)) ([ecbf17b](ecbf17b)) * **color-picker:** Replaces thumb focus outline to rounded ([#7378](#7378)) ([d803980](d803980)) * **filter:** Add filter method ([#7127](#7127)) ([5a4283f](5a4283f)) * **flow:** Adds setFocus method ([#7252](#7252)) ([2472c58](2472c58)) * Improve focus behavior in components ([#7277](#7277)) ([ad9fbca](ad9fbca)) * **input-time-zone:** Add input-time-zone component ([#6947](#6947)) ([87bd496](87bd496)) * **list:** Add slots for filter actions ([#7183](#7183)) ([da07ab1](da07ab1)) * **list:** Add support for dragging items. ([#7109](#7109)) ([7324f70](7324f70)) * **menu-item:** Update spacing and icon layout ([#7381](#7381)) ([5659671](5659671)) * **navigation-logo:** Increase font-size of heading with no description ([#7081](#7081)) ([355e101](355e101)) * **switch:** Updates focus outline to be rounded ([#7390](#7390)) ([2616b82](2616b82)) * **text-area:** Provide additional context for AT users when character limit exceeds ([#7299](#7299)) ([c5678eb](c5678eb)) * **text-area:** Provide additional context for AT users when character limit exceeds ([#7412](#7412)) ([c1af3c7](c1af3c7)) ### Bug Fixes * **accordion, accordion-item:** `icon-position`, `icon-type`, `selection-mode` and `scale` can now be set as props or attributes ([#7191](#7191)) ([2b09aba](2b09aba)) * **action-bar:** No longer delegates focus when clicked on non-focusable region ([#7310](#7310)) ([1a9c15c](1a9c15c)) * **action:** Correctly focus the button after rendering updates. ([#7255](#7255)) ([40fe2ce](40fe2ce)) * **block:** Loader now appears for all loading cases ([#7303](#7303)) ([5af3600](5af3600)) * **block:** Removes extra loading indicator ([#7239](#7239)) ([a334a75](a334a75)) * **card:** Ensure teardown logic is called when disconnected ([#7289](#7289)) ([d07e322](d07e322)) * **chip:** Disconnect mutation observer when component is disconnected from the DOM ([#7418](#7418)) ([412e5fb](412e5fb)) * **color-picker:** Draw slider thumbs within bounds ([#7398](#7398)) ([2f37854](2f37854)) * **color-picker:** Fix opacity slider keyboard nudging ([#7400](#7400)) ([2b4f7c3](2b4f7c3)) * **color-picker:** Maintains correct numbering system when entering invalid RGB value ([#7327](#7327)) ([8d2a3a5](8d2a3a5)) * **combobox:** Add space after grouped items ([#7302](#7302)) ([b1580c7](b1580c7)) * **dropdown-item:** Provides accessible label when href is not parsed ([#7316](#7316)) ([966b83d](966b83d)) * **flow:** Call setFocus() on back button click ([#7285](#7285)) ([9102aa4](9102aa4)) * **input-date-picker:** Provides placeholder text context for AT users ([#7320](#7320)) ([31e0ba2](31e0ba2)) * **input-date-picker:** Reset active date picker date after closing ([#7219](#7219)) ([91b2a1b](91b2a1b)) * **input, input-number:** No longer removes trailing decimal separator ([#7159](#7159)) ([01535cf](01535cf)) * **link:** Adds outline-offset to avoid overlapping with text. ([#7342](#7342)) ([c30db4e](c30db4e)) * **list:** Changing filterText property will now update filtered items ([#7133](#7133)) ([a9c0bce](a9c0bce)) * **list:** Fix keyboard navigation after a listItem's disabled or closed property changes ([#7275](#7275)) ([91d28eb](91d28eb)) * **list:** Fix keyboard navigation when filterEnabled is true ([#7385](#7385)) ([41a2e42](41a2e42)) * **menu-item:** Prevent duplicate border in nested vertical menu-items ([#7387](#7387)) ([186a738](186a738)) * **panel:** Remove double border styling when content isn't provided ([#7368](#7368)) ([91a0610](91a0610)) * Remove style modifying all host components with hidden attribute ([#7346](#7346)) ([3103e2f](3103e2f)) * **scrim:** Update loader scale on resize of component. ([#7419](#7419)) ([24e7f70](24e7f70)) * **slider:** Prevent excessive tick rendering ([#7421](#7421)) ([c799409](c799409)) * **switch:** Fix for focus outline style in certain cases ([#7414](#7414)) ([217324f](217324f)) * **tab-title:** Add full focus outline to closable tab button in high contrast mode ([#7272](#7272)) ([d812d17](d812d17)) * **tooltip:** Avoid extra before open/close event emitting ([#7422](#7422)) ([dbb6818](dbb6818)) * **tooltip:** Deprecate the label property due to the description coming from the component's content ([#7247](#7247)) ([7934d75](7934d75)) * **tooltip:** Emits `close` and `beforeClose` events when container is set to `display:none` ([#7258](#7258)) ([60a4683](60a4683)) * **tooltip:** Ensure --calcite-app-z-index-tooltip is applied ([#7345](#7345)) ([a9a7072](a9a7072)) </details> <details><summary>@esri/calcite-components-react: 1.5.0</summary> ## [1.5.0](https://github.com/Esri/calcite-design-system/compare/@esri/[email protected]...@esri/[email protected]) (2023-08-03) ### Features * Automatically import and define Calcite Components when importing their React wrapper ([#7185](#7185)) ([bf0ff67](bf0ff67)) ### Dependencies * The following workspace dependencies were updated * dependencies * @esri/calcite-components bumped from ^1.5.0-next.38 to ^1.5.0 </details> --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). --------- Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

fix(input): no longer remove trailing decimal separator

59b5a85

github-actions bot assigned anveshmekala Jun 8, 2023

github-actions bot added the bug Bug reports for broken functionality. Issues should include a reproduction of the bug. label Jun 8, 2023

anveshmekala changed the title ~~fix(input): no longer remove trailing decimal separator~~ fix(input): no longer removes trailing decimal separator Jun 8, 2023

benelan reviewed Jun 9, 2023

View reviewed changes

anveshmekala added 12 commits June 15, 2023 19:28

add localized leading decimal zero values

4613212

Merge branch 'master' into anveshmekala/7039-fix-calcite-input-decima…

f241c17

…l-separator

Merge branch 'master' into anveshmekala/7039-fix-calcite-input-decima…

387db5b

…l-separator

Merge branch 'master' into anveshmekala/7039-fix-calcite-input-decima…

54b7143

…l-separator

fix tests

5d6f507

fix tests

c6a1e71

Merge branch 'master' into anveshmekala/7039-fix-calcite-input-decima…

cffe278

…l-separator

cleanup

1c60d07

Merge branch 'master' into anveshmekala/7039-fix-calcite-input-decima…

eacde2d

…l-separator

refactor

c447239

refactor util method

7a0480a

remove redundant code blocks

87f15d7

anveshmekala marked this pull request as ready for review June 21, 2023 23:11

anveshmekala requested a review from a team as a code owner June 21, 2023 23:11

anveshmekala added the pr ready for visual snapshots Adding this label will run visual snapshot testing. label Jun 21, 2023

benelan reviewed Jun 22, 2023

View reviewed changes

anveshmekala marked this pull request as draft June 22, 2023 21:25

anveshmekala added 4 commits June 23, 2023 17:56

refactor util method to handle comma decimal separator

63dff04

Merge branch 'master' into anveshmekala/7039-fix-calcite-input-decima…

d06003b

…l-separator

refactor util methods

6751aaa

update tests

688b98a

anveshmekala marked this pull request as ready for review June 26, 2023 20:12

anveshmekala requested a review from benelan June 26, 2023 20:12

anveshmekala added 3 commits June 27, 2023 11:42

fix failing spec tests

cd4a3ac

use regex

98c86d4

add more spec tests

ce04bab

anveshmekala requested a review from benelan June 27, 2023 22:02

anveshmekala added 2 commits June 27, 2023 17:31

remove unused imports

ecbc697

Merge branch 'master' into anveshmekala/7039-fix-calcite-input-decima…

fbc3831

…l-separator

anveshmekala added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Jun 27, 2023

anveshmekala added 2 commits June 28, 2023 11:09

simplify util method logic

de31f7c

Merge branch 'master' into anveshmekala/7039-fix-calcite-input-decima…

05b771c

…l-separator

anveshmekala added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Jun 28, 2023

jcfranco changed the title ~~fix(input): no longer removes trailing decimal separator~~ fix(input, input-number): no longer removes trailing decimal separator Jun 28, 2023

jcfranco approved these changes Jun 28, 2023

View reviewed changes

benelan approved these changes Jun 28, 2023

View reviewed changes

anveshmekala added 3 commits June 29, 2023 15:43

feedback changes and more e2e tests added

c507400

remove unused methods in number spec test

4a57130

add new lines in test

dfc9311

anveshmekala added pr ready for visual snapshots Adding this label will run visual snapshot testing. and removed pr ready for visual snapshots Adding this label will run visual snapshot testing. labels Jun 29, 2023

anveshmekala merged commit 01535cf into master Jun 29, 2023

anveshmekala deleted the anveshmekala/7039-fix-calcite-input-decimal-separator branch June 29, 2023 23:01

github-actions bot added this to the 2023 July Priorities milestone Jun 29, 2023

This was referenced Jun 29, 2023

chore: release latest #7237

Closed

chore: release latest #7261

Merged

benelan mentioned this pull request Aug 17, 2023

[Tooling] Improve numberStringFormatter helper to handle style:"percent" option #7399

Open

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

anveshmekala commented Jun 8, 2023 •

edited

Loading

benelan Jun 9, 2023

benelan Jun 9, 2023

anveshmekala Jun 9, 2023 •

edited

Loading

benelan Jun 14, 2023

anveshmekala Jun 15, 2023

anveshmekala Jun 15, 2023

benelan Jun 22, 2023

benelan left a comment •

edited

Loading

anveshmekala commented Jun 21, 2023

benelan left a comment

benelan Jun 22, 2023

benelan Jun 22, 2023

benelan Jun 22, 2023

benelan Jun 22, 2023

benelan Jun 22, 2023

benelan Jun 22, 2023

anveshmekala Jun 22, 2023

benelan Jun 24, 2023 •

edited

Loading

anveshmekala commented Jun 28, 2023

jcfranco left a comment

jcfranco Jun 28, 2023

jcfranco commented Jun 28, 2023

anveshmekala commented Jun 28, 2023

benelan left a comment •

edited

Loading

benelan Jun 28, 2023 •

edited

Loading

benelan Jun 28, 2023 •

edited

Loading

anveshmekala Jun 29, 2023

benelan Jun 28, 2023

benelan Jun 29, 2023

anveshmekala Jun 29, 2023 •

edited

Loading

benelan Jun 28, 2023

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

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

Conversation

anveshmekala commented Jun 8, 2023 • edited Loading

Summary

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anveshmekala Jun 9, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benelan left a comment • edited Loading

Choose a reason for hiding this comment

anveshmekala commented Jun 21, 2023

benelan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benelan Jun 24, 2023 • edited Loading

Choose a reason for hiding this comment

anveshmekala commented Jun 28, 2023

jcfranco left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jcfranco commented Jun 28, 2023

anveshmekala commented Jun 28, 2023

benelan left a comment • edited Loading

Choose a reason for hiding this comment

benelan Jun 28, 2023 • edited Loading

Choose a reason for hiding this comment

benelan Jun 28, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anveshmekala Jun 29, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anveshmekala commented Jun 8, 2023 •

edited

Loading

anveshmekala Jun 9, 2023 •

edited

Loading

benelan left a comment •

edited

Loading

benelan Jun 24, 2023 •

edited

Loading

benelan left a comment •

edited

Loading

benelan Jun 28, 2023 •

edited

Loading

benelan Jun 28, 2023 •

edited

Loading

anveshmekala Jun 29, 2023 •

edited

Loading