Feat/update csv download #857
Conversation
lissavxo
force-pushed
the
feat/update-csv-download
branch
from
dba2c8d to
8a0b023
Compare
Klakurka
left a comment
Klakurka
left a comment
There was a problem hiding this comment.
- USD/CAD is going to be an account setting. What I meant here was to have BCH and XEC split out.
- The dropdown label appears in the dropdown list but isn't clickable:
- you can just add a hiddenattribute. - Cursor should be the pointer when hovering over the dropdown buttons.
There was a problem hiding this comment.
- Only show a currency in the dropdown if the button actually has an address for that currency.
- Only show the dropdown at all if the button has addresses for 2 or more currencies.
- The filename when selection 'All Currencies' has two
-instead of 1. - Let's change the filename to [button name]-transactions instead of [button id]-all-transactions (or [button id]-xec-transactions when XEC-only is selected).
There was a problem hiding this comment.
Looks good, only some small changes in the code.
As for the pointer cursor in the dropdown, it would require to refactor the dropdown as something other than an actual <select> dropdown (such as a list of <li> elements) because using CSS to set the cursor as pointer in an actual dropdown doesn't work: the dropdown is rendered by the operating system, not the browser.
We could just ignore it but TBH I think would be way nicer UX if the cursor was really a pointer, since the experience of selecting something that is actually a button feels weird.
pages/button/[id].tsx
Outdated
| try { | ||
| const response = await fetch(`/api/paybutton/download/transactions/${paybutton.id}`) | ||
| let url = `/api/paybutton/download/transactions/${paybutton.id}` | ||
| const isCurrencyEmpty = (value: string): boolean => (value === '') |
There was a problem hiding this comment.
This abstraction seems overly complicated since it's only used once, here right after its defined.
Why not use a simple if (currency === '') below?
There was a problem hiding this comment.
because I am using it in two places and I changed a bit to add undefined check too
Klakurka
left a comment
Klakurka
left a comment
There was a problem hiding this comment.
Looks good to me.
We just need a way to get CAD working even if it's by manually setting it in the db for the account that generates the CSV.
| const networkId = await getNetworkIdFromSlug(slug ?? NETWORK_TICKERS.ecash) | ||
| networkIdArray = [networkId] | ||
| } | ||
| const transactionsGrouped = await fetchTransactionsByPaybuttonIdGroupedByNetwork(paybutton.id, networkIdArray) |
There was a problem hiding this comment.
Why do you need them grouped if you just ungroup it afterwards? This variable is not used anywhere further.
There was a problem hiding this comment.
this is to separate/group by network
services/transactionService.ts
Outdated
| return transactions | ||
| } | ||
|
|
||
| export async function fetchTransactionsByPaybuttonIdGroupedByNetwork ( |
There was a problem hiding this comment.
This is created to be used in a place where the grouping is not relevant. I don't get why it exists, but in any case would be better if it took the txs list, not the button and networkids as a parameter.
There was a problem hiding this comment.
grouping is relevant, we order by date and group by network.. this way we separate the txs list by network
Related to #848
Description
Added a selector in CSV export to split the transaction by currency
Test plan