-
Notifications
You must be signed in to change notification settings - Fork 60
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
Feat/transaction history #863
Conversation
@tobi-bams please the Withdraw text does not have any padding |
@tobi-bams also fix the merge conflicts |
This is because the name field is displaying a |
@tobi-bams is that because it's pulling from fake data? We have names stored in our backend. I also don't see a receiver info in the screenshot raph posted. |
Yes, that's the reason. But Raphael said users using lnauth have their Pubkey as their name. Also, for deposit and Withdrawal, we don't have receivers. Only payment has receivers. |
@tobi-bams ah that makes sense. Thanks for explaining. Let me sync with Milan and get back to you soon. |
@tobi-bams I have an update. Let's restrict the width of the displayed name by width. Then let's have a tooltip pop up that displays the whole name. I've highlighted an example in the image below. You can find this in figma here. |
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.
lgtm just small issues that I can deal with myself will try to merge today
export const formatSat = (sat: number) => { | ||
if (sat === 0 || !sat) { | ||
return '0'; | ||
} | ||
const satsWithComma = sat.toLocaleString(); | ||
const splittedSat = satsWithComma.split(','); | ||
return splittedSat.join(' '); | ||
}; |
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 want to write tests for this but I don't think we have jest working yet
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 want to write tests for this but I don't think we have jest working yet
@tobi-bams Why not use this function in helpers.ts convertToLocaleString
instead of creating a new function
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.
@elraphty convertToLocaleString
is returning something else compared to what I want.
type={'checkbox'} | ||
checked={filter.payment} | ||
onChange={() => handleFilter('payment')} | ||
/>{' '} |
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.
not sure why {' '}
is here
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.
Hm my editor always adds this, I will remove it manually
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.
Hm my editor always adds this, I will remove it manually
type={'checkbox'} | ||
checked={filter.deposit} | ||
onChange={() => handleFilter('deposit')} | ||
/>{' '} |
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.
not sure why {' '} is here
type={'checkbox'} | ||
checked={filter.withdraw} | ||
onChange={() => handleFilter('withdraw')} | ||
/>{' '} |
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.
not sure why {' '} is here
Please don't merge, currently pushing a fix for lnurk users |
@kevkevinpal you can merge now I also removed the empty strings @ecurrencyhodler I also added a tool tip when a users name is so long |
@tobi this looks really good and I can easily click on bounties. Is there a limit to viewing the txn history such amount of rows? I can't seem to see deposits into the org. |
@tobi-bams okay the date seems to pull the time the bounty was created but we need it to display the time it was paid. Also it'd be great if we can get the format in MM/DD/YYYY. I'll create a separate issue. |
@tobi this looks really good and I can easily click on bounties. There are no limits to the number of rows for now, you can't see previous deposits because @elraphty had to create a new table on the backend to accommodate the new design and he is yet to write a migration script to populate the new table with previous data. |
@elraphty please can you return the date the bounty was paid?? |
Okay thanks @tobi-bams. I actually think we should keep the no limit for now. |
* feat: transaction history table new design * update: updated UI with more style * feat: filter payments by * update: payment history display properly * update: payment history filter * feat: make table responsive * feat: format sats to display as in design * update: added format name function to shorten lnauth user name * feat: added a tooltip for users with pubkey as username * update: removed empty strings * fix: fixed some eslint issue
Describe your changes
Issue ticket number and link
Type of change
Please delete options that are not relevant.
Checklist before requesting a review