-
Notifications
You must be signed in to change notification settings - Fork 46
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.
lgtm 👍
@@ -0,0 +1,213 @@ | |||
import * as React from 'react'; |
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.
super nit pick but there's a couple unused variables in this file
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.
Yeah sorry 😅 I still need to push the lint fixes
}) => { | ||
const quill = useQuill(); | ||
const transactions = useCell(quill.cells.transactions); | ||
const data = |
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.
Is the idea that we would eventually query the data on chain to get transactions or will we always just look at local storage?
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.
For now we can just look at the local transactions as the on-chain tx could include txs from more than 1 wallet as part of a batch. It would be a little bit difficult to parse the individual actions per wallet but certainly is doable and could be worked upon in future! This will be particularly useful when the user is migrating keys from either a burner wallet or changing browsers.
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.
Nice that makes sense. Thanks for the explanation!
</div> | ||
); | ||
}, | ||
// eslint-disable-next-line react/no-unstable-nested-components |
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.
any suggestion on how to fix this? tried a couple of things. Added a skip comment to make CI happy.
@blakecduncan @voltrevo
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.
Based on https://github.com/jsx-eslint/eslint-plugin-react/blob/master/docs/rules/no-unstable-nested-components.md and some local testing I did, you need to extract that cell to another functional component in this same file:
// Not super happy with the typing of 'cell' here but it works.
const TransactionTableCell = (cell: { getValue: () => string }) => {
return (
<div
className={[
'bg-blue-100',
'bg-opacity-40',
'px-3',
'm-auto',
'flex',
'items-center',
'gap-2',
'active:bg-opacity-70',
'cursor-pointer',
'text-blue-600',
'rounded-full',
'w-max',
].join(' ')}
{...onAction(() =>
window.open(`https://etherscan.io/tx/${cell.getValue()}`),
)}
>
{cell.getValue()}
<ArrowUpRight />
</div>
);
};
And then consume it in the cell:
// other columns
...
{
header: 'Tx Hash',
accessorFn: (_) => {
const placeholder =
'0xfbe7276011b411d474c5ec224e50912b5bab77f72f6294585a3064962496178';
return formatCompactAddress(placeholder);
},
cell: TransactionTableCell,
},
...
Full diff:
diff --git a/extension/source/Home/Wallet/Wallets/TransactionsTable.tsx b/extension/source/Home/Wallet/Wallets/TransactionsTable.tsx
index 5b879e4..c4abfcb 100644
--- a/extension/source/Home/Wallet/Wallets/TransactionsTable.tsx
+++ b/extension/source/Home/Wallet/Wallets/TransactionsTable.tsx
@@ -44,6 +44,33 @@ export const TableHeader: React.FunctionComponent<ITransactionsTable> = ({
);
};
+const TransactionTableCell = (cell: { getValue: () => string }) => {
+ return (
+ <div
+ className={[
+ 'bg-blue-100',
+ 'bg-opacity-40',
+ 'px-3',
+ 'm-auto',
+ 'flex',
+ 'items-center',
+ 'gap-2',
+ 'active:bg-opacity-70',
+ 'cursor-pointer',
+ 'text-blue-600',
+ 'rounded-full',
+ 'w-max',
+ ].join(' ')}
+ {...onAction(() =>
+ window.open(`https://etherscan.io/tx/${cell.getValue()}`),
+ )}
+ >
+ {cell.getValue()}
+ <ArrowUpRight />
+ </div>
+ );
+};
+
export const TransactionsTable: React.FunctionComponent<ITransactionsTable> = ({
selectedAddress,
}) => {
@@ -88,31 +115,7 @@ export const TransactionsTable: React.FunctionComponent<ITransactionsTable> = ({
'0xfbe7276011b411d474c5ec224e50912b5bab77f72f6294585a3064962496178';
return formatCompactAddress(placeholder);
},
- // eslint-disable-next-line react/no-unstable-nested-components
- cell: (t) => (
- <div
- className={[
- 'bg-blue-100',
- 'bg-opacity-40',
- 'px-3',
- 'm-auto',
- 'flex',
- 'items-center',
- 'gap-2',
- 'active:bg-opacity-70',
- 'cursor-pointer',
- 'text-blue-600',
- 'rounded-full',
- 'w-max',
- ].join(' ')}
- {...onAction(() =>
- window.open(`https://etherscan.io/tx/${t.getValue()}`),
- )}
- >
- {t.getValue()}
- <ArrowUpRight />
- </div>
- ),
+ cell: TransactionTableCell,
},
{
header: 'Actions',
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.
The underlying reason for this is that on every re-render, your cell component function will be re-created and re-rendered every time the table updates, which can be very bad for performance. This is similar to what happens when you assign an anonymous function in a component.
// The function in onClick will be re-created every render, potentially causing unnecessary renders in the Button component
<Button onCick={() => { alert("surprise!"); }} />
// The same happens here as 'handleClick' is re-created on every render.
const handleClick = () => { alert("surprise!"); };
<Button onCick={handleClick} />
// With React.useCallback, the only thing that will cause the Button to re-render
// is a hook/internal state change, or the useCallback hook deps if any were included.
const handleClick = React,useCallback(() => { alert("surprise!"); }, []);
<Button onCick={handleClick} />
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.
Yeah gotcha! I was trying to pass props explicitly to the component after extracting it out like this
cell: (t) => <TransactionTableCell actions={t.getValue()} />
which wasn't working
8d953dd
to
d30c692
Compare
What is this PR doing?
Follow-up to this PR in the linked issue
How can these changes be manually tested?
Does this PR resolve or contribute to any issues?
Checklist
Guidelines
resolve conversation
button is for reviewers, not authors