-
Notifications
You must be signed in to change notification settings - Fork 9
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 407 inputs logic #2572
base: dev
Are you sure you want to change the base?
fix APP 407 inputs logic #2572
Conversation
✅ Deploy Preview for terrasos ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
✅ Deploy Preview for regen-website ready!
To edit notification comments on pull requests, go to your Netlify site configuration. |
@erikalogie @blushi see testing instructions Note: I've tried to mimic the behaviour in the video shared in the Jira task but since we are using an |
This looks much better. One thing that is a little weird is that if I type in 0 decimals and then keep typing past 2 digits, it erases the decimals that I typed. Whereas with other numbers, the expected behavior of simply not being able to type more digits happens. Screen.Recording.2024-12-12.at.9.22.52.AM.mov |
5d55063
to
df367f1
Compare
@erikalogie have a look please. I have updated the code to allow typing two decimals that are zero, but in some scenarios the number may be converted to just |
This looks great now! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, just a few nits.
Just one small difference I noticed (that was already there btw) between the editable input in the order summary card and the CreditsInput
and CurrencyInput
is that within the editable input, I cannot use a comma as a decimal separator which is a convention that is widely used: https://en.wikipedia.org/wiki/Decimal_separator#/media/File:DecimalSeparator.svg, looks like we could update sanitizeValue
function to account for that, but can be in a separate issue if you want?
web-marketplace/src/components/molecules/CreditsAmount/CreditsInput.tsx
Outdated
Show resolved
Hide resolved
// Allow only two decimals when they are all zeros | ||
const value = event.target.value; | ||
const decimalPart = value.split('.')?.[1]; | ||
if ( | ||
decimalPart && | ||
decimalPart.length > 2 && | ||
decimalPart.startsWith('00') && | ||
paymentOption === PAYMENT_OPTIONS.CARD | ||
) { | ||
event.target.value = value.slice(0, -1); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we could refactor to use a function to get the if
value, as the same check is also used in handleInput
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
While they are not exactly the same, refactoring makes sense, thanks!
…t and using card payment
df367f1
to
8cf4080
Compare
After having a look, I've created a new issue for this because it'll require a bit more work than just update |
Description
https://regennetwork.atlassian.net/browse/APP-407
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...
How to test
https://deploy-preview-2572--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...