Skip to content

Signature-Request refactor#19104

Merged
cryptotavares merged 39 commits intodevelopfrom
refactor-signature-request
Jul 20, 2023
Merged

Signature-Request refactor#19104
cryptotavares merged 39 commits intodevelopfrom
refactor-signature-request

Conversation

@NiranjanaBinoy
Copy link
Copy Markdown
Contributor

@NiranjanaBinoy NiranjanaBinoy commented May 11, 2023

Fixes: #17245

Explanation

Refactored https://github.com/MetaMask/metamask-extension/blob/develop/ui/components/app/signature-request/signature-request.component.js component:

  1. Removed the container component
  2. Converted signature-request.component.js functional component named signature-request.js
  3. Updated the component to use the new design components.
  4. Added the use of useSelectors to query state and minimized the props it takes.
  5. Add test coverage and update the storybook

Screenshots/Screencaps

Before

Screenshot 2023-05-11 at 3 56 49 PM

After

Screenshot 2023-05-24 at 3 22 25 PM

Manual Testing Steps

  1. From the test Dapp initiates a Sign typed Data V3 or sign typed Data v4.
  2. Make sure the correct page pop up and the signature request is working

Pre-merge author checklist

  • I've clearly explained:
    • What problem this PR is solving
    • How this problem was solved
    • How reviewers can test my changes
  • Sufficient automated test coverage has been added

Pre-merge reviewer checklist

  • Manual testing (e.g. pull and build branch, run in browser, test code being changed)
  • PR is linked to the appropriate GitHub issue
  • IF this PR fixes a bug in the release milestone, add this PR to the release milestone

If further QA is required (e.g. new feature, complex testing steps, large refactor), add the Extension QA Board label.

In this case, a QA Engineer approval will be be required.

@NiranjanaBinoy NiranjanaBinoy added the team-confirmations-secure-ux-PR PRs from the confirmations team label May 11, 2023
@NiranjanaBinoy NiranjanaBinoy self-assigned this May 11, 2023
@github-actions
Copy link
Copy Markdown
Contributor

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.

@NiranjanaBinoy NiranjanaBinoy changed the title Refactoring Signature-Request Signature-Request refactor May 11, 2023
@github-actions github-actions Bot added area-UI Relating to the user interface. PS-team PS MM extension team issues labels May 11, 2023
@NiranjanaBinoy NiranjanaBinoy marked this pull request as ready for review May 12, 2023 15:17
@NiranjanaBinoy NiranjanaBinoy requested a review from a team as a code owner May 12, 2023 15:17
@NiranjanaBinoy NiranjanaBinoy added area-transactions and removed area-UI Relating to the user interface. PS-team PS MM extension team issues labels May 12, 2023
@NiranjanaBinoy NiranjanaBinoy requested review from digiwand and jpuri and removed request for montelaidev May 12, 2023 15:19
@github-actions github-actions Bot added area-UI Relating to the user interface. PS-team PS MM extension team issues labels May 12, 2023
@NiranjanaBinoy NiranjanaBinoy removed area-UI Relating to the user interface. area-transactions PS-team PS MM extension team issues labels May 12, 2023
@github-actions github-actions Bot added area-UI Relating to the user interface. PS-team PS MM extension team issues labels May 12, 2023
Copy link
Copy Markdown
Contributor

@georgewrmarshall georgewrmarshall left a comment

Choose a reason for hiding this comment

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

Hey @NiranjanaBinoy, this might be out of scope of this ticket but you could use a few more component-library components which would bring this component up to date with the UI :)

Comment thread ui/components/app/signature-request/signature-request.js Outdated
Comment thread ui/components/app/signature-request/signature-request.js Outdated
Copy link
Copy Markdown
Contributor

@georgewrmarshall georgewrmarshall left a comment

Choose a reason for hiding this comment

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

Left some suggestions. I would even suggest leaving out the h tags if it passes e2e tests they don't seem semantically correct to me. It's probably fine for the default p tag to be used

Comment thread ui/components/app/signature-request/__snapshots__/signature-request.test.js.snap Outdated
Comment thread ui/components/app/signature-request/signature-request.js Outdated
Comment thread ui/components/app/signature-request/signature-request.js Outdated
Comment thread ui/components/app/signature-request/signature-request.js Outdated
Comment thread ui/helpers/utils/transactions.util.js Outdated
Comment thread ui/components/app/signature-request/signature-request.js Outdated
Comment thread ui/components/app/signature-request/signature-request.js Outdated
Comment thread ui/components/app/signature-request/signature-request.js
Comment thread ui/components/app/signature-request/signature-request.js
Comment thread ui/components/app/signature-request/signature-request.js Outdated
@NiranjanaBinoy NiranjanaBinoy force-pushed the refactor-signature-request branch from dfa866e to e865101 Compare July 19, 2023 22:48
@metamaskbot
Copy link
Copy Markdown
Collaborator

Builds ready [f087b93]
Page Load Metrics (1597 ± 51 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint1063371444823
domContentLoaded14391874159710651
load14391874159710651
domInteractive14391874159710651
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: -3.13 KiB (-0.04%)
  • common: 0 Bytes (0.00%)

@NiranjanaBinoy NiranjanaBinoy removed DO-NOT-MERGE Pull requests that should not be merged needs-qa Label will automate into QA workspace labels Jul 20, 2023
@seaona
Copy link
Copy Markdown
Member

seaona commented Jul 20, 2023

the PR looks good from QA side @NiranjanaBinoy 🔥

@cryptotavares cryptotavares merged commit 0c203d0 into develop Jul 20, 2023
@cryptotavares cryptotavares deleted the refactor-signature-request branch July 20, 2023 17:51
@github-actions github-actions Bot locked and limited conversation to collaborators Jul 20, 2023
@metamaskbot metamaskbot added the release-10.36.0 Issue or pull request that will be included in release 10.36.0 label Jul 20, 2023
@Gudahtt Gudahtt added release-11.1.0 Issue or pull request that will be included in release 11.1.0 and removed release-10.36.0 Issue or pull request that will be included in release 10.36.0 labels Sep 19, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

QA Passed release-11.1.0 Issue or pull request that will be included in release 11.1.0 team-confirmations-secure-ux-PR PRs from the confirmations team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Refactor ui/components/app/signature-request/

10 participants