Skip to content

feat: format numbers payments#986

Merged
chedieck merged 8 commits intomasterfrom
feat/format-amount-payments
Jun 12, 2025
Merged

feat: format numbers payments#986
chedieck merged 8 commits intomasterfrom
feat/format-amount-payments

Conversation

@lissavxo
Copy link
Copy Markdown
Collaborator

Related to #985

Description

Added decimals to crypto amount in payments page.

Test plan

Check amount numbers on payments page.

@Klakurka Klakurka requested review from Klakurka and chedieck March 25, 2025 08:51
Copy link
Copy Markdown
Member

@Klakurka Klakurka left a comment

Choose a reason for hiding this comment

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

Comma isn't working for me...
image

Also the space is now missing between the crypto amount and fiat value:

image

I was kinda thinking we could split these up into 2 columns:

  • Amount
  • Value ([fiat ticker])

thoughts?

@lissavxo lissavxo force-pushed the feat/format-amount-payments branch from 7e3058c to 6699ac6 Compare March 25, 2025 15:29
@lissavxo lissavxo requested a review from Klakurka March 26, 2025 18:48
Copy link
Copy Markdown
Member

@Klakurka Klakurka left a comment

Choose a reason for hiding this comment

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

Unformatted amount is being appended to beginning of formatted amount:

image

@lissavxo lissavxo force-pushed the feat/format-amount-payments branch from 967abfe to 7821b30 Compare March 27, 2025 19:27
@lissavxo lissavxo requested review from Klakurka March 27, 2025 19:28
Copy link
Copy Markdown
Member

@Klakurka Klakurka left a comment

Choose a reason for hiding this comment

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

The sorting doesn't work right. If you sort by value, it will give you all of one currency first, then the other (eg. XEC first, then BCH).

@Klakurka
Copy link
Copy Markdown
Member

I'm not sure if this is related to this branch but I get this error when sorting by amount DESC:

image

@lissavxo lissavxo closed this Apr 3, 2025
@lissavxo lissavxo reopened this May 8, 2025
Klakurka
Klakurka previously approved these changes May 28, 2025
Copy link
Copy Markdown
Collaborator

@chedieck chedieck left a comment

Choose a reason for hiding this comment

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

This implementation of repeating certain info like the values and networkId in a single column of the row is really confusing and unecessary.
Check TanStack/table#571

Copy link
Copy Markdown
Member

@Klakurka Klakurka left a comment

Choose a reason for hiding this comment

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

Sorting by 'button' on /payments breaks the site.

image

@lissavxo
Copy link
Copy Markdown
Collaborator Author

Sorting by 'button' on /payments breaks the site.

image

Should be good now

@lissavxo lissavxo force-pushed the feat/format-amount-payments branch from fc0762c to f1fe51e Compare May 30, 2025 20:00
@lissavxo lissavxo requested review from Klakurka May 30, 2025 20:00
Klakurka
Klakurka previously approved these changes Jun 3, 2025
values,
amount: tx.amount
amount: tx.amount,
networkId: tx.address.networkId
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Still repeating info here

redis/types.ts Outdated
@@ -48,11 +48,18 @@ export interface ButtonDisplayData {
export interface AmountData {
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

  1. AmountData is the same of what you're now calling Amount except for the values.
  2. in the Payment interface we have values: AmountData, making this even more confusing
  3. AmountData is exported even though is not used anywhere.
  4. You're adding another column (amount) to Payment, even though all that info is already there

This is super confusing and almost impossible to follow up. That is probably the reason why redundant columns keep stacking upon each other.

There certainly shouldn't be a Amount and AmountData interface. It has no reason to have networkId on it either, since that is also already present in the parent interface. If you need to take something from it, and if that actually makes sense, then use Omit.

@lissavxo lissavxo requested a review from chedieck June 10, 2025 14:47
@lissavxo lissavxo force-pushed the feat/format-amount-payments branch from 7950a9b to 927df25 Compare June 10, 2025 17:22
@chedieck chedieck merged commit 8a6cc55 into master Jun 12, 2025
2 checks passed
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