Skip to content

fix: APP-540 add support for commas in editable input#2598

Merged
r41ph merged 4 commits intodevfrom
fix-APP-540-add-support-for-commas-in-editable-input
Feb 26, 2025
Merged

fix: APP-540 add support for commas in editable input#2598
r41ph merged 4 commits intodevfrom
fix-APP-540-add-support-for-commas-in-editable-input

Conversation

@r41ph
Copy link
Contributor

@r41ph r41ph commented Feb 12, 2025

Description

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

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. Verify that the decimal and thousands separators reflect the user's location in:
  • Order summary card in the editable input and the prices => buy Credits flow step 2
  • In the credits and currency inputs => buy Credits flow step 1
  • Buy Credits flow success modal
  • My Orders page
  • ... maybe we should standardise this and use the right numbers format based on location all across the marketplace, wdyt?

While working on this I've been changing the browser's language (in Chrome in Settings > Language > Preferred languages) and putting different languages on top makes the numbers format change. I haven't been able to see this settings update work on the buy credits flow step 1, although I think @blushi can see the French local format (commas) in step 1, is that right?

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)

@netlify
Copy link

netlify bot commented Feb 12, 2025

Deploy Preview for regen-website ready!

Name Link
🔨 Latest commit de868d6
🔍 Latest deploy log https://app.netlify.com/sites/regen-website/deploys/67bef3c7c3811100085ec85c
😎 Deploy Preview https://deploy-preview-2598--regen-website.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

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

@netlify
Copy link

netlify bot commented Feb 12, 2025

Deploy Preview for terrasos ready!

Name Link
🔨 Latest commit de868d6
🔍 Latest deploy log https://app.netlify.com/sites/terrasos/deploys/67bef3c73bb719000828523e
😎 Deploy Preview https://deploy-preview-2598--terrasos.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

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

@r41ph
Copy link
Contributor Author

r41ph commented Feb 12, 2025

@erikalogie @S4mmyb see testing instructions

@r41ph r41ph requested a review from blushi February 12, 2025 13:37
@erikalogie
Copy link
Collaborator

@r41ph how can I test this, I used my VPN to connect to a place in France but it doesn't seem to work

@r41ph
Copy link
Contributor Author

r41ph commented Feb 13, 2025

@erikalogie I believe is the browser's setting language that is checked for this not the location. The browser is the one handling the numbers formatting (in the cases where they do it, because some browsers/versions just let HTML handle it, which means they don't show commas)

@blushi
Copy link
Member

blushi commented Feb 17, 2025

maybe we should standardise this and use the right numbers format based on location all across the marketplace, wdyt?

yes definitely, could you create a task for that?

Copy link
Member

@blushi blushi left a comment

Choose a reason for hiding this comment

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

image

Now when I type 1,1 (=1.1) in the currency field, the credits are set to 1,11 and vice versa while in this case $1 = 1 credit

@blushi
Copy link
Member

blushi commented Feb 20, 2025

image

Now when I type 1,1 (=1.1) in the currency field, the credits are set to 1,11 and vice versa while in this case $1 = 1 credit

any fix for this @r41ph ?

@r41ph
Copy link
Contributor Author

r41ph commented Feb 24, 2025

Now when I type 1,1 (=1.1) in the currency field, the credits are set to 1,11 and vice versa while in this case $1 = 1 credit

It doesn't seem to be related with these updates. I see this happening in dev too: https://dev.app.regen.network/project/C01-007/buy

@blushi I have created a new issue to fix it: https://regennetwork.atlassian.net/browse/APP-612

@erikalogie
Copy link
Collaborator

Ok I tested with my browser's language set to Spanish from Spain and it worked as expected.

@erikalogie
Copy link
Collaborator

I just moved APP-612 into this sprint

@r41ph r41ph requested a review from blushi February 24, 2025 14:53
Copy link
Member

@blushi blushi left a comment

Choose a reason for hiding this comment

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

pre-approving


const amountValid = +currentValue <= maxValue && +currentValue > 0;
const amountValid =
+currentValue.replace(/,/g, '.') <= maxValue &&
Copy link
Member

Choose a reason for hiding this comment

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

maybe we could set up some util function to not have to repeat .replace(/,/g, '.') several times?

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 have store the value in a variable, wdyt?

@r41ph r41ph force-pushed the fix-APP-540-add-support-for-commas-in-editable-input branch from 70fc523 to 2aff969 Compare February 25, 2025 15:52
@r41ph r41ph requested a review from blushi February 25, 2025 15:52
@r41ph r41ph force-pushed the fix-APP-540-add-support-for-commas-in-editable-input branch from 2aff969 to fa90392 Compare February 26, 2025 09:49
@r41ph r41ph force-pushed the fix-APP-540-add-support-for-commas-in-editable-input branch from fa90392 to de868d6 Compare February 26, 2025 10:58
@r41ph r41ph merged commit 232e4cc into dev Feb 26, 2025
18 checks passed
@r41ph r41ph deleted the fix-APP-540-add-support-for-commas-in-editable-input branch February 26, 2025 11:15
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