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

fix: confirm page header should show address in fullscreen #10467

Merged
merged 1 commit into from
Feb 17, 2021

Conversation

shanejonas
Copy link
Contributor

@shanejonas shanejonas commented Feb 17, 2021

Fixes: #9920

Explanation:
This PR fixes a regression in logic where it was previously using a renderTop function to decide on rendering the page headers, and always rendering children. This PR brings back that functionality of always rendering the children, instead of returning null in some cases.

Manual testing steps:

  1. Use the metamask test-dapp to create a token
  2. Use metamask in fullscreen
  3. Send a token to any address
  4. Confirm page is missing the sender to recipient addresses

image

@shanejonas shanejonas requested a review from a team as a code owner February 17, 2021 00:18
@shanejonas shanejonas requested a review from danjm February 17, 2021 00:18
@github-actions
Copy link
Contributor

github-actions bot commented Feb 17, 2021

CLA Signature Action: All authors have signed the CLA. You may need to manually re-run the blocking PR check if it doesn't pass in a few minutes.

@shanejonas
Copy link
Contributor Author

I have read the CLA Document and I hereby sign the CLA

@metamaskbot
Copy link
Collaborator

Builds ready [369b2ed]
Page Load Metrics (581 ± 9 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint43775794
domContentLoaded535618579199
load537619581199
domInteractive535617579199

@shanejonas shanejonas self-assigned this Feb 17, 2021
@danjm
Copy link
Contributor

danjm commented Feb 17, 2021

This solution looks good! Code is correct and great unit test addition. I have QA'd this in the browser.

A note for posterity (that does not need to be addressed in this PR). If we trace down the source of the logic, it looks like the showEdit prop is false if either of two conditions are met. Either (A) the confirmation is for a token approval or contract deployment, or (B) it is for a transaction which has a lastGasPrice on its local txMeta data. I think (B) can never be true while on a the confirmation screen. This possibility was eliminated long ago when we moved from the old retryTransaction method to the distinct speed up and cancel transaction methods and we handled them through UIs other than this page. A task for a different PR is to remove logic related to that case (see isTxReprice in ui/app/components/pages/confirm-transaction-base/confirm-transaction-base.component.js). I'll also note that token approval and contract deployment continue to behave correctly.

@shanejonas shanejonas merged commit f4819e4 into develop Feb 17, 2021
@shanejonas shanejonas deleted the fix/confirm-page-header-show-address branch February 17, 2021 02:06
@github-actions github-actions bot locked and limited conversation to collaborators Feb 17, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No sender and recipient addresses shown when sending a token in full screen mode
3 participants