Fixes #3425: Better support for batch transactions #5437
Conversation
| // if (isEtherTransaction) { | ||
| // const pathname = `${CONFIRM_TRANSACTION_ROUTE}/${id}${CONFIRM_SEND_ETHER_PATH}` | ||
| // return <Redirect to={{ pathname }} /> | ||
| // } |
There was a problem hiding this comment.
Instead of commenting this out we should be able to remove it
| @@ -32,24 +32,24 @@ export default class ConfirmTransactionSwitch extends Component { | |||
| txData, | |||
| methodData: { name }, | |||
| fetchingData, | |||
| isEtherTransaction, | |||
| // isEtherTransaction, | |||
There was a problem hiding this comment.
Instead of commenting this out we should be able to remove it
There was a problem hiding this comment.
Awesome, I left it in for the review, it has been removed now. I outlined why I removed this code here.
|
@PaddyMc this looks great - pagination between transactions works as expected, and I like the addition of the "go to first" or "go to last" buttons. I've asked @cjeria to take a look to see if there's a slightly smoother visual element we can use 😄 One thing I noticed when testing on this random dapp. Transactions are ordered well, and I can skip through them all. But if I reject the first transaction, it almost always then shows the last transaction of the queue (rather than the next oldest tx). As per this comment - don't worry about the old UI, we plan to remove it entirely very soon. Thanks for your patience! |
|
@bdresser Thanks, I'm glad you like it! Feel free to ping me when the UX review is completed and I'll complete all the changes requested! Below is listed the changes that I will make, feel free to add anymore you think of as well:
|
PaddyMc
force-pushed
the
better-support-for-batch-transactions
branch
from
a68c03f to
2a93a0d
Compare
|
@bdresser Just an fyi, this was updated a few days ago and is ready for review! |
|
Hey @PaddyMc, sorry for the delay in reviewing this. I'm seeing two bits of unexpected behavior, and I'm sorry if we got our wires crossed (1) for regular transactions, the order they display in the extension and popup are different. I'd expect them to be the same. For example,
(2) batch transactions aren't being ordered correctly
Does all this make sense? Again, sorry for the slow back and forth, and please let me know if you have questions. (cc @whymarrh ) |
|
No problem at all @bdresser . Thanks for the feedback, I'll update the PR in a few days. Did you think the spacing for the navigation container and the network container was ok? |
|
Thanks @PaddyMc, this is a solid improvement! |
|
Np @whymarrh , happy to help out! |
MetaMask would sometimes get into a state where the notification popup would never open. This could happen if the notification window was closed shortly after being opened. After this happened, no popups would show up until after the extension was reset. This was happening because the background thought the popup was already open. The variable it uses to track whether the popup was open or not was being set to `true` immediately after the background asked the browser to open a new window, before a handler was attached that could respond to the window being closed. Removing this line seems to solve the problem. This line was added originally in #5437, which dealt with batch transactions. Batches of transactions seem to work just fine without this line though (from local testing), and I can't think of why this would be required. Closes #7051
MetaMask would sometimes get into a state where the notification popup would never open. This could happen if the notification window was closed shortly after being opened. After this happened, no popups would show up until after the extension was reset. This was happening because the background thought the popup was already open. The variable it uses to track whether the popup was open or not was being set to `true` immediately after the background asked the browser to open a new window, before a handler was attached that could respond to the window being closed. Removing this line seems to solve the problem. This line was added originally in #5437, which dealt with batch transactions. Batches of transactions seem to work just fine without this line though (from local testing), and I can't think of why this would be required. Closes #7051
This is a backport of #8314. Here's the original description: MetaMask would sometimes get into a state where the notification popup would never open. This could happen if the notification window was closed shortly after being opened. After this happened, no popups would show up until after the extension was reset. This was happening because the background thought the popup was already open. The variable it uses to track whether the popup was open or not was being set to `true` immediately after the background asked the browser to open a new window, before a handler was attached that could respond to the window being closed. Removing this line seems to solve the problem. This line was added originally in #5437, which dealt with batch transactions. Batches of transactions seem to work just fine without this line though (from local testing), and I can't think of why this would be required. Closes #7051
This is a backport of #8314. Here's the original description: MetaMask would sometimes get into a state where the notification popup would never open. This could happen if the notification window was closed shortly after being opened. After this happened, no popups would show up until after the extension was reset. This was happening because the background thought the popup was already open. The variable it uses to track whether the popup was open or not was being set to `true` immediately after the background asked the browser to open a new window, before a handler was attached that could respond to the window being closed. Removing this line seems to solve the problem. This line was added originally in #5437, which dealt with batch transactions. Batches of transactions seem to work just fine without this line though (from local testing), and I can't think of why this would be required. Closes #7051
Fixes #3425