Skip to content

Feat/merge addresses button detail#998

Merged
chedieck merged 7 commits intomasterfrom
feat/merge-addresses-button-detail
May 15, 2025
Merged

Feat/merge addresses button detail#998
chedieck merged 7 commits intomasterfrom
feat/merge-addresses-button-detail

Conversation

@lissavxo
Copy link
Copy Markdown
Collaborator

@lissavxo lissavxo commented Apr 14, 2025

Related to #800 #943

Description

Merged transactions in a single table and added a column with the address in the button details screen.

Test plan

  • Go to button details page and check if transactions are showing up as expected
  • Test if the screen is automatically updated when a new transaction arrives

@lissavxo lissavxo force-pushed the feat/merge-addresses-button-detail branch from 5661236 to 98088ad Compare April 14, 2025 17:26
@Klakurka Klakurka requested a review from chedieck April 16, 2025 00:03
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.

Alignment of columns Amount, Network, Tx and Address is wrong

image

const orderDescString: Prisma.SortOrder = orderDesc ? 'desc' : 'asc'

let orderByQuery
if (orderBy !== undefined && orderBy !== '') {
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.

Can you add some comments for what this code block is doing?

@lissavxo
Copy link
Copy Markdown
Collaborator Author

Good catch, fixed the table and added comment

@lissavxo lissavxo requested a review from chedieck April 28, 2025 18:41
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.

Ordering works an it's aligned, but the little gray arrow now always shows besides the "Address" column header, despite it being ordered by another column, e.g. "Date".

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.

The name of this component should change now, since the behavior changed.

PaybuttonTransactions is more appropriate.

@lissavxo lissavxo requested a review from chedieck May 9, 2025 19:42
chedieck
chedieck previously approved these changes May 15, 2025
@chedieck chedieck self-requested a review May 15, 2025 13:38
@chedieck chedieck merged commit 755a7a4 into master May 15, 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.

2 participants