Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add Estimated time to pending tx #6924

Merged
merged 21 commits into from
Nov 1, 2019
Merged

Add Estimated time to pending tx #6924

merged 21 commits into from
Nov 1, 2019

Conversation

Krist14n
Copy link
Contributor

This fix includes the estimated time remaining for pending transactions #6850

@danjm
Copy link
Contributor

danjm commented Jul 29, 2019

@Krist14n Don't worry about the failing test-integration-flat tests. We will be getting rid of them soon any way. So it is not worth spending time to fix them. I will figure out what to do with them and perhaps push a commit to your branch.

danjm
danjm previously requested changes Jul 29, 2019
View reviewed changes
Copy link
Contributor

@danjm danjm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is close. I left some comments regarding where exactly we should make the fetchBasicGasAndTimeEstimates and how we might improve performance a bit. Also see my note regarding failing tests (which I will do something about).

ui/app/components/app/transaction-time-remaining/transaction-time-remaining.component.js Outdated Show resolved Hide resolved
ui/app/components/app/transaction-time-remaining/transaction-time-remaining.container.js Outdated Show resolved Hide resolved
ui/app/components/app/transaction-time-remaining/transaction-time-remaining.container.js Outdated Show resolved Hide resolved
@danjm
Copy link
Contributor

danjm commented Aug 2, 2019

Code looks good. Now just need to get the tests passing.

@Krist14n Krist14n requested a review from frankiebee as a code owner August 6, 2019 23:39
@Krist14n Krist14n force-pushed the estimated-time-pending-tx branch 2 times, most recently from 8100288 to 8c11cf0 Compare August 7, 2019 18:09
@danjm danjm added this to the UI Sprint 20 [Sept 16] milestone Sep 16, 2019
@danjm danjm self-assigned this Sep 16, 2019
@danjm danjm force-pushed the estimated-time-pending-tx branch from b502511 to 67fa5ad Compare October 10, 2019 17:13
@Gudahtt Gudahtt force-pushed the estimated-time-pending-tx branch from 9ed4c9c to 4ca924d Compare October 17, 2019 19:22
@bdresser
Copy link
Contributor

heads up @tmashuang we should put this through a lot of QA -- if these estimates are frequently way off, we will need to tweak or shelve the feature.

also, probably want to show < 30s once the estimate gets down there so we avoid showing 0s for tx that are still pending.

@danjm danjm force-pushed the estimated-time-pending-tx branch 2 times, most recently from ff1959a to b18f5ad Compare October 23, 2019 10:41
Gudahtt
Gudahtt requested changes Oct 23, 2019
View reviewed changes
Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There appears to be an empty file added: ui/app/components/app/transaction-time-remaining/index.scss

ui/app/components/app/transaction-list-item/transaction-list-item.container.js Outdated Show resolved Hide resolved
ui/app/components/app/transaction-list/transaction-list.component.js Outdated Show resolved Hide resolved
ui/app/components/app/transaction-list/transaction-list.component.js Outdated Show resolved Hide resolved
ui/app/components/app/transaction-list/transaction-list.container.js Outdated Show resolved Hide resolved
ui/app/components/app/transaction-time-remaining/transaction-time-remaining.component.js Outdated Show resolved Hide resolved
ui/app/components/app/transaction-list/transaction-list.component.js Outdated Show resolved Hide resolved
@danjm
Copy link
Contributor

danjm commented Oct 28, 2019

@Gudahtt I addressed your comments in 74b1204df

@danjm danjm force-pushed the estimated-time-pending-tx branch 2 times, most recently from 0bfda44 to d788005 Compare October 28, 2019 20:31
Gudahtt
Gudahtt previously approved these changes Oct 28, 2019
View reviewed changes
@danjm danjm force-pushed the estimated-time-pending-tx branch 2 times, most recently from 2a3e85d to 1a4d021 Compare October 29, 2019 15:52
@danjm danjm force-pushed the estimated-time-pending-tx branch from 1a4d021 to b578d80 Compare October 29, 2019 16:27
Gudahtt
Gudahtt previously approved these changes Oct 29, 2019
View reviewed changes
Copy link
Member

@Gudahtt Gudahtt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

whymarrh
whymarrh suggested changes Oct 30, 2019
View reviewed changes
ui/app/helpers/utils/gas-time-estimates.util.js Outdated
@@ -0,0 +1,85 @@
import BigNumber from 'bignumber.js'
window.BigNumber = BigNumber
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this necessary?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in ff192618d

ui/app/helpers/utils/gas-time-estimates.util.js Outdated
const formattedSec = `${seconds ? seconds + ' sec' : ''}`
const formattedCombined = formattedMin && formattedSec
? `${symbol}${formattedMin} ${formattedSec}`
: symbol + [formattedMin, formattedSec].find(t => t)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[formattedMin, formattedSec].find(t => t) seems a bit weird—is there a better way we could express this?

Maybe formattedMin || formattedSec? Is that the same thing?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done in ff192618d

@danjm
Copy link
Contributor

danjm commented Oct 31, 2019

I have updated this PR to remove unneeded and misleading code, and improved the UX by actually having the time remaining estimate count down seconds. Additionally, the countdown results in a '< 30 s' time estimate once the estimate goes below 30 seconds.

Here are some videos demoing the UX: https://streamable.com/s517x, https://streamable.com/ljgg4

@danjm danjm force-pushed the estimated-time-pending-tx branch from ff19261 to 625412c Compare October 31, 2019 23:00
whymarrh
whymarrh approved these changes Nov 1, 2019
View reviewed changes
Copy link
Contributor

@whymarrh whymarrh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thanks @danjm (and @Krist14n!)

@danjm danjm merged commit f9cd775 into MetaMask:develop Nov 1, 2019
tmashuang pushed a commit that referenced this pull request Nov 4, 2019
@Krist14n @tmashuang
Add Estimated time to pending tx (#6924)
64e72ba 
* Add estimated time to pending transactions

* add sytles for pending transactions component

* add media queries styling for pending transactions component

* fix lint errors, remove extra spaces

* refactor code to call `fetchBasicGasAndTimeEstimates` method once

* refactor code to call `getgetRenderableTimeEstimate` method once

* fix, correct export to use `transaction-time-remaining-component`

* fix indentation issues after running `yarn lint`

* newBigSigDig in gas-price-chart.utils supports strings

* Code cleanup

* Ensure fetchBasicGasAndTimeEstimates is only called from tx-list if there are pending-txs

* Move gas time estimate utilities into utility file

* Move getTxParams to transaction selector file

* Add feature flag for display of remaining transaction time in tx history list

* Fix circular dependency by removing unused import of transactionSelector in selectors.js

* Use correct feature flag property name transactionTime

* Ensure that tx list component correctly responds to turning tx time feature on

* Prevent precision errors in newBigSigDig

* Code clean up for pending transaction times

* Update transaction-time-remaining feature to count down seconds, countdown seconds and show '< 30'

* Code clean up for transaction-time-remaining feature
@bbondy bbondy mentioned this pull request Dec 16, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@danjm danjm danjm left review comments

@whymarrh whymarrh whymarrh approved these changes

@Gudahtt Gudahtt Gudahtt approved these changes

@frankiebee frankiebee Awaiting requested review from frankiebee

Assignees

@danjm danjm

Labels
None yet
Projects
None yet
Milestone
UI Sprint 23
Development

Successfully merging this pull request may close these issues.
5 participants
@Krist14n @danjm @bdresser @whymarrh @Gudahtt