-
Notifications
You must be signed in to change notification settings - Fork 82
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
frontend: refactor transaction.tsx #2776
frontend: refactor transaction.tsx #2776
Conversation
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.
partly reviewed
frontends/web/src/components/transactions/components/details.tsx
Outdated
Show resolved
Hide resolved
import { CopyableInput } from '../../copy/Copy'; | ||
import transactionStyle from '../transactions.module.css'; |
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.
With #2781 merged, we should start using absolute imports for imports that are ../
or higher ('../../
etc..). Thanks 👍
c193a1b
to
127baba
Compare
127baba
to
a545ce5
Compare
@thisconnect rebased and applied all suggestions. the commit messages should change too for some commits now, but I'll do that after everything is reviewed. |
e591e9d
to
fff08b9
Compare
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.
I have some suggestions but generally looks good!
transactionInfo.current.fee && transactionInfo.current.fee.amount ? ( | ||
<TxDetail | ||
label={t('transaction.fee')} | ||
title={feeRatePerKb.amount ? feeRatePerKb.amount + ' ' + feeRatePerKb.unit + '/Kb' : ''} |
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.
Might as well since we're here: for consistency, let's use template literal instead of string concat here (and in other places).
const parseTimeShort = (time: string, lang: string) => { | ||
const options = { | ||
year: 'numeric', | ||
month: 'numeric', | ||
day: 'numeric', | ||
} as Intl.DateTimeFormatOptions; | ||
return new Date(Date.parse(time)).toLocaleString(lang, options); | ||
}; |
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.
Can we put this in a utils
file?
<div className={`${parentStyle.detail} ${parentStyle.addresses}`}> | ||
<label>{label}</label> | ||
<div className={parentStyle.detailAddresses}> | ||
{values.map((addr_or_txid) => ( |
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.
Again, for consistency, let's use camelCase here e.g addrOrTxId
instead of snake_case
.
import { TxDateDetail } from './date'; | ||
import { TxStatusDetail } from './status'; | ||
import { TxAddressOrTxID } from './address-or-txid'; | ||
import parentStyle from '../transaction.module.css'; |
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.
Let's use absolute import here :)
3ff44ca
to
00156a5
Compare
@shonsirsha thanks! rebased/applied chagnes, PTAL |
ed32a83
to
78c5f5b
Compare
@shonsirsha I added one commit to fix the problem that all transactions are fetched immediately instead of when the details dialog opens, this can be in the |
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.
Code LGTM (some nits). I'll test in a bit
}: TProps) => { | ||
const { t } = useTranslation(); | ||
|
||
const [transactionInfo, setTransactionInfo] = useState<ITransaction | null>(); |
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.
transactionInfo
here has the (implicit) type of ITransaction | null | undefined
.
nit: Should we just do the following:
const [transactionInfo, setTransactionInfo] = useState<ITransaction | null>(); | |
const [transactionInfo, setTransactionInfo] = useState<ITransaction | null>(null); |
? 🤔
Since getTransaction
returns either ITransaction | null
anyway (not undefined
), at least from its type.
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.
yes definitely
borderLess | ||
flexibleHeight | ||
className={parentStyle.detailAddress} | ||
value={addrOrTxID} /> |
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.
nit:
value={addrOrTxID} /> | |
value={addrOrTxID} | |
/> |
😄
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 tested. 👍
I don't have that many transactions, feel free if you'd like to test as well @thisconnect
78c5f5b
to
8380b96
Compare
(addressed nits) |
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.
please also take a look at the commits, it looks to me that all "extract" commits should be squashed into 1 commit.
and the 2 last commits into 1 commit I made a comment
frontends/web/src/components/transactions/components/show-details-button.tsx
Show resolved
Hide resolved
frontends/web/src/components/transactions/components/status.tsx
Outdated
Show resolved
Hide resolved
transactionInfo.fee && transactionInfo.fee.amount ? ( | ||
<TxDetail | ||
label={t('transaction.fee')} | ||
title={feeRatePerKb.amount ? feeRatePerKb.amount + ' ' + feeRatePerKb.unit + '/Kb' : ''} |
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.
I can't see any title here.
Tried master, mainnet/ testnet and looked at send, receive and send to self tx's but I could never see feeRatePerKb.amount in this title attribute.
I guess this was a bit a hidden feature for debuggin purpose which broke at some point. Backend sends empy string.
I think we can just remove this title, but one day we should show fee/vbyte if available.
addresses, | ||
txid, | ||
}: TPropsAddressOrTxID) => { | ||
const values = addresses ? addresses : [txid]; |
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.
How about naming this component: <TxDetailCopyableValues values={[]} />
and make values mandatory array instead of optional addresses and txid.
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.
Oh yes, makes sense now after splitting up into TxAddress
and TxAddressOrTxID
49d487c
to
a97f4ca
Compare
@thisconnect thanks :) addressed everything, PTAL |
a97f4ca
to
ebfa671
Compare
2
Outdated
day: 'numeric', | ||
} as Intl.DateTimeFormatOptions; | ||
return new Date(Date.parse(time)).toLocaleString(lang, options); | ||
}; |
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.
remove? :)
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.
I have no idea how this happened, sorry.
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.
no problem, maybe quickly check your own PR after pushing, then you'd see such accidental commits.
Rename sDate to shortDate in the transaction component, for a more descriptive, easier to understand name.
Use template literals for JSX element classes instead of joining a list for a more concice and cleaner style in the transaction.tsx component.
Define the parseTimeShort helper function outside of the transaction.tsx component so that it is not re-defined on every render. This improves performance and makes the component cleaner.
Create TxDetail component in transactions/components that provides the default html structure/layout which is used by most details in the transaction details dialog. This makes the dialog more concise and easier to understand.
ebfa671
to
d905e5c
Compare
Extract reusable sub-components from the transaction.tsx component to make the transaction component cleaner and easier to understand. Extracted subcomponents: - tx. direction arrow - tx. date - tx. status - tx. show details button - tx. address and copyable values
Extract the transactions details dialog in transactions.tsx into its own component to make the transaction component itself more concice and easier to understand.
d905e5c
to
cbbc794
Compare
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 thanks
Refactor the transaction.tsx component to be more concise and easier to understand, also fix some minor nits like using template literals for element class names.
The PR implements all suggestions from #2549 and does not do anything else. The commits can be reviewed own their own. But the first and the third could be skipped because the changed code is moved in other commits.
The last commit moves, and changes the transaction dialog to its own component which is why everything editing something in
Dialog
prior to that commit can also be reviewed later in the last commit.Please note that the many additions are caused by the license and other overhead from extracting sub-components. I think the refactor is almost addition/deletion neutral without those.