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

Add nonce to transaction details #8716

Merged
merged 1 commit into from
Jun 1, 2020
Merged

Conversation

brad-decker
Copy link
Contributor

#8564 removed the nonce from the transaction list item UI as per the design, but the design intended for the nonce to be relocated into the new transaction details view. #8564 used an incremental approach for implementing new designs and simply moved the details into a popover but did not implement the new UI. Therefore, the nonce was erroneously removed from the user's view.

This PR is extending the old details UI to add the nonce to it until I can implement the new design.

@brad-decker brad-decker requested a review from a team as a code owner June 1, 2020 20:47
@brad-decker brad-decker force-pushed the add-nonce-to-details branch from 0c24834 to 5f7d60e Compare June 1, 2020 20:59
const { gas, gasPrice, value, className, nativeCurrency, showFiat, totalInHex, gasUsed } = this.props

const { gas, gasPrice, value, className, nonce, nativeCurrency, showFiat, totalInHex, gasUsed } = this.props
console.log(nonce)
Copy link
Member

Choose a reason for hiding this comment

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

Leftover console.log here

Suggested change
console.log(nonce)

className="transaction-breakdown__value"
value={nonce}
/>
) : '?'
Copy link
Member

Choose a reason for hiding this comment

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

I would recommend using "Unknown" here, since we already have it as a localized message

Suggested change
) : '?'
) : t('unknown')

Copy link
Member

Choose a reason for hiding this comment

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

Or just leave it blank, perhaps? 🤔

Suggested change
) : '?'
) : null

@brad-decker brad-decker force-pushed the add-nonce-to-details branch from 5f7d60e to 411f399 Compare June 1, 2020 21:33
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!

@metamaskbot
Copy link
Collaborator

Builds ready [411f399]
Page Load Metrics (607 ± 62 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint31111472110
domContentLoaded36477060512962
load36577160712962
domInteractive36477060512962

@brad-decker brad-decker merged commit c8de0b7 into develop Jun 1, 2020
@brad-decker brad-decker deleted the add-nonce-to-details branch June 1, 2020 22:19
Gudahtt added a commit that referenced this pull request Jun 1, 2020
* origin/develop:
  Fix connect long URL styling (#8719)
  Add nonce to transaction details (#8716)
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.

3 participants