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: APP-404 update field and trigger validation instead of using onChange #2514

Open
wants to merge 3 commits into
base: dev
Choose a base branch
from

Conversation

r41ph
Copy link
Contributor

@r41ph r41ph commented Oct 24, 2024

Description

https://regennetwork.atlassian.net/browse/APP-404

Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • provided a link to the relevant issue or specification
  • provided instructions on how to test
  • reviewed "Files changed" and left comments if necessary
  • confirmed all CI checks have passed

How to test

  1. Go to the Choose Credits step in the Buy Credits flow and verify that the currency input field doesn't throw an error when changing the value
  2. https://deploy-preview-2514--regen-marketplace.netlify.app/project/mai-ndombe-4/buy

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items
.

I have...

  • confirmed all author checklist items have been addressed
  • reviewed code correctness and readability
  • verified React components follow DRY principles
  • reviewed documentation is accurate
  • reviewed tests
  • manually tested (if applicable)

Copy link

netlify bot commented Oct 24, 2024

👷 Deploy Preview for regen-website processing.

Name Link
🔨 Latest commit 4b2f4bd
🔍 Latest deploy log https://app.netlify.com/sites/regen-website/deploys/671a76e68de5380008dc5545

@r41ph r41ph requested a review from blushi October 24, 2024 13:28
@r41ph
Copy link
Contributor Author

r41ph commented Oct 24, 2024

@erikalogie see testing instructions

@erikalogie
Copy link
Collaborator

LGTM

const currency = useWatch({
control,
name: CURRENCY,
});

const handleOnChange = useCallback(
(event: ChangeEvent<HTMLInputElement>) => {
// Remove zeros in non decimal values and update the value
const value = event.target.value;
if (!value.includes('.')) setValue(CURRENCY_AMOUNT, Number(value));
Copy link
Member

Choose a reason for hiding this comment

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

removing this introduces another bug, you can't type a number with 2 decimals if the first decimal is 0, eg 21,05

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverting doesn't seem to work either. There seems to be an error that may be related to stripe when introducing numbers 2.01. Any idea what the thrown error is about @blushi . Also the user gets stuck in the error screen because the amount introduced is in the Local Storage.

Screen.Recording.2024-10-24.at.16.30.26.mov

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 that should be a separated issue btw. If we are happy with the fix for the current issue we may want to merge this so other features don't have to deal with not being able to use the currency input.

Copy link
Member

@blushi blushi Oct 24, 2024

Choose a reason for hiding this comment

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

Stripe doesn't allow amounts with more than 2 decimals for USD
we provide to stripe the amount in cents, so for 2.01 it should be 201 but I guess when we do 2.01*100 in JS it gives 200.99999999999997 because of its floating point representation, this is why in some places I use .toFixed(...), we should use it in this case as well

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok, thanks - I have added a possible solution to this too

@r41ph r41ph requested a review from blushi October 24, 2024 16:17
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.

3 participants