Fix input field bugs #166

gianfra-t · 2024-09-19T20:41:33Z

Fixes #164.

Additionally, it fixes:

Crash when backspace is tapped multiple times, causing input empty string.
Management of trailing zeros.

netlify · 2024-09-19T20:41:49Z

✅ Deploy Preview for pendulum-pay ready!

Name	Link
🔨 Latest commit	`59a937c`
🔍 Latest deploy log	https://app.netlify.com/sites/pendulum-pay/deploys/6706926fd5f9cb00080dc37f
😎 Deploy Preview	https://deploy-preview-166--pendulum-pay.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

TorstenStueber

Thanks. This works almost perfectly.

Edge cases:

e.g., text is 123.45 and I mark the part "3.4" and press "." I expect it to be replaced by 12.5, but nothing happens
the input field allows to paste anything nonsensical (text, numbers with two ".", etc) – in this case the calculation of debouncedAmountBigDecimal has an unhandled exception as it expects that the input form is always a valid number (we could wrap this into a try catch)

It is fine the way it is, already a big improvement. Therefore, I approve.

If it does not make too much effort, the above two points could possibly be addresses by not preventing default in handleOnKeyPress (remove this handler altogether) and just normalizing all text in handleOnInput (i.e., filter out everything outside [0-9.] and then remove any additional . after the first one and then remove extraneous decimals).

src/components/NumericInput/index.tsx

TorstenStueber · 2024-09-20T17:18:53Z

src/pages/swap/index.tsx

@@ -128,7 +128,7 @@ export const SwapPage = () => {
    } else {
      form.setValue('toAmount', '');
    }
-  }, [form, tokenOutData.data, toToken]);
+  }, [form, tokenOutData.data, toToken, inputAmountIsStable]);


Usually it is considered bad practice to add variables to the dependency list that don't occur inside the function. Why is this necessary here?

True! the issue is the following scenario:

Let's say in the field we have 12 and we add . at the end. I noticed that this won't execute this effect, therefore not setting setIsQuoteSubmitted(false); which leads to the quote being disabled.

Now I don't understand why this is the case, since the form state changes and should trigger this, but it didn't. Therefore I decided to add inputAmountIsStable which will always go from false to true after a change in input.

Do you think we can make an exception? or shall we explore why form doesn't get updated?

I ended up removing the use of isQuoteSubmitted here. I think the loading property of tokenOutData should be enough to mark toAmount as disabled. WDYT? This also solves the bug I mentioned above.
Only this modification was needed.

I think it makes sense that the effect was not executed. setIsQuoteSubmitted was always set to true whenever the input field changed. But when the input field changes in a way that the output amount calculated through Nabla does not change (as is the case when just adding a .), then the effect will not be triggered to reset setIsQuoteSubmitted to false and the output field stays disabled.

I think it is exactly right to only use the loading property of tokenOutData as this correctly reflects that the output amount is currently being determined. So nicely done.

src/pages/swap/index.tsx

TorstenStueber · 2024-09-20T20:38:56Z

src/components/NumericInput/index.tsx

+      target.value.slice(0, firstDotOccurence + 1) + target.value.slice(firstDotOccurence + 1).replace('.', '');
+  }
+
+  if (exceedsMaxDecimals(target.value, maxDecimals)) {
    target.value = target.value.slice(0, -1);


Here we should not just remove one final digit but all extraneous digits at once (otherwise this whole function will be called repeatedly and each time only one extra digit will be removed until no more digits can be removed and target.dispatchEvent will not be called anymore).

Suggested change

target.value = target.value.slice(0, -1);

target.value = target.value.slice(0, firstDotOccurence + 1 + maxDecimals);

Should work because firstDotOccurence is defined whenever exceedsMaxDecimals is true.

TorstenStueber · 2024-09-20T20:39:27Z

src/components/NumericInput/index.tsx

+  target.value = target.value.replace(/,/g, '.').replace(/[^0-9.]/g, '');
+
+  // Handle case where user tries to add a second decimal point, we keep the leftmost one.
+  let firstDotOccurence = target.value.search(/[.]/);


Let's change to const (linter complains).

Suggested change

let firstDotOccurence = target.value.search(/[.]/);

const firstDotOccurence = target.value.search(/[.]/);

Sharqiewicz

@TorstenStueber Thanks for noticing these edge cases for NumericInput. I think they definitely need to be added to NumericInput.test.tsx in Portal. I think we should make the changes in Portal as well. I have created a PR that fixes these errors and creates test cases here: pendulum-chain/portal#561. Can we just copy this into Vortex?

ebma · 2024-09-23T09:46:24Z

While unfortunate, I agree with @Sharqiewicz that it would be better first to fix the NumericInput field in the portal and then copy it over. The implementation used in the prototype originates from the portal but must be a version added prior to some improvements added to the portal input in the meantime. The input sanitization is overall easier to read in the NumericInput component of the portal IMO.

gianfra-t · 2024-09-23T11:59:43Z

@Sharqiewicz @ebma I'm okay to wait for the portal changes and then modify this. I think it still may be a bit different given how the consumers of the form in Vortex handle the value.

For instance, this part was not my first choice, yet it was important, otherwise the form would receive the uncleaned string (which lead to the empty string error when deleting fully the amount, for instance).

gianfra-t · 2024-09-23T12:00:39Z

@TorstenStueber I will wait to address your last comments until we have consensus on the previous comment.

ebma · 2024-10-02T13:49:48Z

We merged the equivalent changes to the portal in pendulum-chain/portal#561. @gianfra-t please copy the src/components/Form/From/NumericInput/helpers.ts and src/components/Form/From/NumericInput/index.tsx files here so we can profit from the same fixes. No need to copy the test as it won't work in this repository due to the lacking jest configuration.

TorstenStueber

Can you please also fix the following bug (which has been introduced before): https://github.com/pendulum-chain/vortex/pull/166/files#diff-777e0c2221f5feeef076c359496045ddd95d6d376a0fc1395b330f8a9b64a5bbR91

Here we also need to check whether tokenOutData.stableAmountInUnits is the empty string. Otherwise the Big constructor throws an error. If it is the empty string, then we can treat inputAmountIsStable as false.

src/components/NumericInput/index.tsx

src/pages/swap/index.tsx

gianfra-t · 2024-10-03T11:59:15Z

Yes, I copied the portal fix but it is not ready yet. As you say it requires a few extra checks to work properly here.

gianfra-t · 2024-10-03T13:23:19Z

@TorstenStueber now it should be good. I added the handling of empty string, only ., and removing extra leading zeros.

TorstenStueber · 2024-10-07T22:25:40Z

src/pages/swap/index.tsx

@@ -1,4 +1,5 @@
 import { useEffect, useMemo, useRef, useState } from 'preact/hooks';
+import { Fragment } from 'preact';


This is not used.

TorstenStueber · 2024-10-07T22:28:45Z

@gianfra-t Thanks for the changes. Can you please still implement this? #166 (review)

gianfra-t · 2024-10-08T12:17:14Z

@TorstenStueber I (re)added the empty string condition here, is it the same issue? tokenOutData.stableAmountInUnits can't be empty if the input field is not, I think.

Nevertheless I added the condition you were referring here.

TorstenStueber

Great. Thanks!

gianfra-t · 2024-10-09T12:05:01Z

Hey @Sharqiewicz, we copied (most) of the sanitizing implementation from the portal as you suggested. Thanks!

…et-dump-always-creating-new-sheet

fix input field bugs

4854d82

remove extra log

bd47eac

TorstenStueber approved these changes Sep 20, 2024

View reviewed changes

src/components/NumericInput/index.tsx Outdated Show resolved Hide resolved

TorstenStueber reviewed Sep 20, 2024

View reviewed changes

gianfra-t added 3 commits September 20, 2024 15:49

improvements to input form

d0635ba

set toAmount to 0 when no data available

84b81ec

stabilize more toAmount display

09073f2

TorstenStueber reviewed Sep 20, 2024

View reviewed changes

src/pages/swap/index.tsx Outdated Show resolved Hide resolved

TorstenStueber reviewed Sep 20, 2024

View reviewed changes

Sharqiewicz requested changes Sep 23, 2024

View reviewed changes

ebma mentioned this pull request Sep 23, 2024

Fix/numericinput edge cases pendulum-chain/portal#561

Merged

2 tasks

replacing fixes with portal implementation

afbacef

TorstenStueber reviewed Oct 2, 2024

View reviewed changes

src/components/NumericInput/index.tsx Outdated Show resolved Hide resolved

src/components/NumericInput/index.tsx Outdated Show resolved Hide resolved

src/pages/swap/index.tsx Outdated Show resolved Hide resolved

gianfra-t added 4 commits October 3, 2024 10:04

handle leading zeros, empty string

3d15ae6

cleanup

47fea00

handle leading zeros on paste

8d85ebc

remove unused function

e9534fe

TorstenStueber reviewed Oct 7, 2024

View reviewed changes

add empty string condition

48c29cd

TorstenStueber approved these changes Oct 9, 2024

View reviewed changes

gianfra-t requested a review from Sharqiewicz October 9, 2024 12:03

ebma added 2 commits October 9, 2024 16:23

Merge branch 'polygon-prototype-staging' into 158-fix-transaction-she…

022edbf

…et-dump-always-creating-new-sheet

Revert faulty change of disabled prop of swap submit button

59a937c

ebma merged commit 3734155 into polygon-prototype-staging Oct 9, 2024
5 checks passed

ebma deleted the 158-fix-transaction-sheet-dump-always-creating-new-sheet branch October 9, 2024 14:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix input field bugs #166

Fix input field bugs #166

gianfra-t commented Sep 19, 2024 •

edited

Loading

netlify bot commented Sep 19, 2024 •

edited

Loading

TorstenStueber left a comment

TorstenStueber Sep 20, 2024

gianfra-t Sep 20, 2024

gianfra-t Sep 20, 2024

TorstenStueber Sep 20, 2024 •

edited

Loading

TorstenStueber Sep 20, 2024

TorstenStueber Sep 20, 2024

Sharqiewicz left a comment

ebma commented Sep 23, 2024

gianfra-t commented Sep 23, 2024

gianfra-t commented Sep 23, 2024

ebma commented Oct 2, 2024

TorstenStueber left a comment •

edited

Loading

gianfra-t commented Oct 3, 2024

gianfra-t commented Oct 3, 2024

TorstenStueber Oct 7, 2024

TorstenStueber commented Oct 7, 2024

gianfra-t commented Oct 8, 2024

TorstenStueber left a comment

gianfra-t commented Oct 9, 2024

	target.value = target.value.slice(0, -1);
	target.value = target.value.slice(0, firstDotOccurence + 1 + maxDecimals);

	let firstDotOccurence = target.value.search(/[.]/);
	const firstDotOccurence = target.value.search(/[.]/);

		@@ -1,4 +1,5 @@
		import { useEffect, useMemo, useRef, useState } from 'preact/hooks';
		import { Fragment } from 'preact';

Fix input field bugs #166

Fix input field bugs #166

Conversation

gianfra-t commented Sep 19, 2024 • edited Loading

netlify bot commented Sep 19, 2024 • edited Loading

✅ Deploy Preview for pendulum-pay ready!

TorstenStueber left a comment

Choose a reason for hiding this comment

TorstenStueber Sep 20, 2024

Choose a reason for hiding this comment

gianfra-t Sep 20, 2024

Choose a reason for hiding this comment

gianfra-t Sep 20, 2024

Choose a reason for hiding this comment

TorstenStueber Sep 20, 2024 • edited Loading

Choose a reason for hiding this comment

TorstenStueber Sep 20, 2024

Choose a reason for hiding this comment

TorstenStueber Sep 20, 2024

Choose a reason for hiding this comment

Sharqiewicz left a comment

Choose a reason for hiding this comment

ebma commented Sep 23, 2024

gianfra-t commented Sep 23, 2024

gianfra-t commented Sep 23, 2024

ebma commented Oct 2, 2024

TorstenStueber left a comment • edited Loading

Choose a reason for hiding this comment

gianfra-t commented Oct 3, 2024

gianfra-t commented Oct 3, 2024

TorstenStueber Oct 7, 2024

Choose a reason for hiding this comment

TorstenStueber commented Oct 7, 2024

gianfra-t commented Oct 8, 2024

TorstenStueber left a comment

Choose a reason for hiding this comment

gianfra-t commented Oct 9, 2024

gianfra-t commented Sep 19, 2024 •

edited

Loading

netlify bot commented Sep 19, 2024 •

edited

Loading

TorstenStueber Sep 20, 2024 •

edited

Loading

TorstenStueber left a comment •

edited

Loading