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.

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

Implement new transaction list design #8564

Merged
merged 16 commits into from
May 26, 2020

Conversation

brad-decker
Copy link
Contributor

@brad-decker brad-decker commented May 9, 2020

Main View ScreenShot image
Demo Videos Demo Full Screen & [Demo Popup Window]
New hooks added - useCancelTransaction: a hook for both determining if a transaction can be canceled (account has enough funds to cover cancelation fee) and creating a memoized function for kicking off the cancelation workflow - useRetryTransaction: a hook for determining the eligibility of retrying a transaction and creating a memoized function to kick off the retry workflow - useI18nContext: a hook that is just a shortcut for importing both `useContext` and `I18nContext` - useMethodData: a hook for tapping into token method data. Includes an effect to make sure that method data is requested if it is not already in `knownMethodData` - useTokenData: a hook for memoized computation of transaction params data on token transfers - useTransactionDisplayData: A hook for analyzing a transaction group and deriving a data shape that can be used for displaying a transaction to the user.

added utility methods

  • formatDateWithYearContext: the new design calls for displaying the transaction year only if it did not occur this year. This method just provides a way for specifying two different format strings, one to use for current year transactions and a fallback to use for previous years transactions

refactored ListItem component

  • Previously the ListItem component was very specific to transactions
  • Moved all of the transaction-related logic into transaction-list-item
  • Added new props to make the list item component more versatile
  • Refactored component to be a stateless functional component

refactored TransactionList component

  • Refactored to a functional component
  • Combined container logic into component using hooks
  • Updated the instantiation of TransactionListItems to match new component API

refactored TransactionListItem component

  • Refactored to a functional component
  • Combined and greatly modified container logic into the component using hooks
  • Prepares all props needed to render a ListItem with transaction data

moved TransactionListItemDetails into a Popover

  • Very minimal changes
  • Dropped some media queries that adapted the detail view to larger display sizes as its always now in a Popover
  • the new design of this view is not yet implemented, will do that in a new PR
  • Modified tests so that the existing tests are run against the first child of the primary wrapper (Runs tests against the original intended target instead of the new Popover)

NEW UX FEATURES

Queued Transactions When users have more than one pending transaction now all items beyond the first that aren't "Unapproved" transactions will have a "Queued" label and will not have the Loading indicator behind them. image
Incremental loading of history As pointed out in #8572 we are eagerly loading transactions into the history view, but we don't need to. We now have an incremental loader that shows 10 items at a time with a view more button to load more. image

updated e2e test selectors for finding currency amounts in the list

Bugs fixed

fixes #8572

@brad-decker brad-decker changed the base branch from develop to use-currency-hooks May 9, 2020 04:10
@brad-decker brad-decker force-pushed the transaction-detail-and-history branch from d8617f5 to cb5b5b4 Compare May 9, 2020 04:11
@brad-decker brad-decker force-pushed the use-currency-hooks branch from 6b5177b to 5ed3ab5 Compare May 11, 2020 11:08
@brad-decker brad-decker force-pushed the transaction-detail-and-history branch from cb5b5b4 to 982911d Compare May 11, 2020 11:16
@brad-decker brad-decker force-pushed the use-currency-hooks branch 5 times, most recently from 382ce91 to aea45cf Compare May 11, 2020 20:23
@whymarrh
Copy link
Contributor

@brad-decker do you think it would be possible to address #8572 here?

@brad-decker
Copy link
Contributor Author

I’ll see what can be done @whymarrh I was thinking the same when I saw the issue report

@brad-decker brad-decker force-pushed the use-currency-hooks branch 5 times, most recently from ef91741 to 08cb87d Compare May 12, 2020 17:31
@brad-decker brad-decker force-pushed the transaction-detail-and-history branch from 982911d to c6281ec Compare May 12, 2020 22:34
@brad-decker
Copy link
Contributor Author

brad-decker commented May 12, 2020

@whymarrh this now includes a resolution for #8572 - transaction-list.component now includes a limit for completed transactions and slices through the array of completed transactions. A new "View More" (pending @rachelcope 's feedback) button at the bottom of the transaction list loads an increment of transactions. I have created a constant PAGE_INCREMENT value of 10 for this purpose, but I expect that we could add a new user preference for this in the future (or perhaps 100 is a more reasonable default?)

pictured with a PAGE_INCREMENT of 1:

image

@brad-decker brad-decker changed the base branch from use-currency-hooks to develop May 12, 2020 22:47
@brad-decker brad-decker force-pushed the transaction-detail-and-history branch 2 times, most recently from ee27a7c to 0b730ed Compare May 12, 2020 23:10
@brad-decker brad-decker changed the base branch from develop to refactor-list-item May 13, 2020 19:23
@brad-decker brad-decker force-pushed the transaction-detail-and-history branch 2 times, most recently from 32d55e8 to 562a1c7 Compare May 13, 2020 20:19
@metamaskbot
Copy link
Collaborator

Builds ready [562a1c7]
Page Load Metrics (623 ± 51 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint318039105
domContentLoaded37173062110651
load37273162310651
domInteractive37072962110651

@brad-decker brad-decker force-pushed the transaction-detail-and-history branch from 562a1c7 to 9e4cd4b Compare May 13, 2020 21:22
@metamaskbot
Copy link
Collaborator

Builds ready [9e4cd4b]
Page Load Metrics (671 ± 52 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint3599512110
domContentLoaded39480666911153
load41480867110952
domInteractive39480666811153

@brad-decker brad-decker force-pushed the transaction-detail-and-history branch from 9e4cd4b to 444aed1 Compare May 13, 2020 21:45
@brad-decker brad-decker marked this pull request as ready for review May 13, 2020 21:47
import { getKnownMethodData } from '../selectors/selectors'

/**
* useMethodData
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: can we add a single line description of the function here instead? Maybe extracted from the larger description below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you like:
* Access known method data and attempt to resolve unknown method data for this?



/**
* useRetryTransaction
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: can we add a single line description of the function here instead? Maybe extracted from the larger description below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the description is now short enough to meet this need. Should I just omit the name? new:

/**
 * Provides a reusable hook that, given a transactionGroup, will return
 * a method for beginning the retry process
 * @param {Object} transactionGroup - the transaction group
 * @return {Function}
 */

*/

/**
* useTransactionDisplayData
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: can we add a single line description of the function here instead? Maybe extracted from the larger description below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

/**
 * Get computed values used for displaying transaction data to a user
 *

import { getConversionRate, getSelectedAccount } from '../selectors'

/**
* useCancelTransaction
Copy link
Contributor

Choose a reason for hiding this comment

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

Nit: can we add a single line description of the function here instead? Maybe extracted from the larger description below.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

/**
 * Determine whether a transaction can be cancelled and provide a method to
 * kick off the process of cancellation.

@brad-decker brad-decker force-pushed the transaction-detail-and-history branch from c702933 to b695366 Compare May 26, 2020 17:58
@metamaskbot
Copy link
Collaborator

Builds ready [b695366]
Page Load Metrics (877 ± 81 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint39117662010
domContentLoaded433106487517082
load441106587716981
domInteractive432106487417082

@metamaskbot
Copy link
Collaborator

Builds ready [7966975]
Page Load Metrics (587 ± 62 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint31116432010
domContentLoaded33972958513062
load34073058713062
domInteractive33872858513062

@brad-decker brad-decker requested review from whymarrh and Gudahtt May 26, 2020 19:58
@brad-decker
Copy link
Contributor Author

@whymarrh and @Gudahtt -- that last change was made and tests passing. 🤞

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.

LGTM!

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.

:shipit:

Thanks for championing this across the finish line! This is a huge improvement. 🎉

@brad-decker brad-decker merged commit 706dc02 into develop May 26, 2020
@brad-decker brad-decker deleted the transaction-detail-and-history branch May 26, 2020 20:49
Gudahtt added a commit that referenced this pull request May 28, 2020
The display logic from this component was inlined into the
`TransactionList` component in #8564.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Account with thousands of transactions loads slowly
4 participants