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

replaced actionablemessage in Import-Token.js #20147

Merged
merged 9 commits into from
Jul 25, 2023

Conversation

pritam1813
Copy link
Contributor

Explanation

Screenshots/Screencaps

Before

before

After

after

Manual Testing Steps

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.

@pritam1813 pritam1813 requested a review from a team as a code owner July 24, 2023 15:59
@github-actions
Copy link
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.

@pritam1813
Copy link
Contributor Author

Hi @georgewrmarshall , please review this.

@codecov
Copy link

codecov bot commented Jul 24, 2023

Codecov Report

Merging #20147 (4a80f56) into develop (3e6f1c3) will decrease coverage by 0.01%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##           develop   #20147      +/-   ##
===========================================
- Coverage    69.45%   69.45%   -0.01%     
===========================================
  Files          984      984              
  Lines        37290    37290              
  Branches     10012    10012              
===========================================
- Hits         25899    25897       -2     
- Misses       11391    11393       +2     
Files Changed Coverage Δ
ui/pages/swaps/import-token/import-token.js 87.50% <ø> (ø)

... and 2 files with indirect coverage changes

Copy link
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 @pritam1813, This is looking great! I have one suggestion and one optional suggestion

ui/pages/swaps/import-token/import-token.js Outdated Show resolved Hide resolved
Comment on lines 6 to 7
import Button from '../../../components/ui/button';
import Box from '../../../components/ui/box';
Copy link
Contributor

Choose a reason for hiding this comment

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

Optional: If you'd like to take this PR a step further you could use the Box and Button components from the component-library as well. You will have to update some of the component props as the component API is different for the Button component so it does add a level of complexity to completing this task. Completely optional!

Copy link
Contributor

Choose a reason for hiding this comment

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

You could also replace UrlIcon with AvatarToken from the component-library

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah sure, why not.This was only a single line change PR anyway. Should I try and remove all the deprecated Components in this file ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that would still make this PR a reasonable size to review. You could also do it in parts if you'd like to get this PR in first. Then create another PR to update the rest.

Copy link
Contributor

Choose a reason for hiding this comment

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

I should point out that the more components that are replaced the more likely hood you will need to update e2e tests which adds a fair amount of complexity to the task.

@georgewrmarshall georgewrmarshall added the team-design-system All issues relating to design system in Extension label Jul 24, 2023
pritam1813 and others added 7 commits July 25, 2023 11:31
…taMask#20148)

* adds listeners for signatureControll and adds the handleSigningEvents method

* clean up

* updates signature request containers files

* adds necessary methods

* wip

* signing flow with core methods

* yarn lint

* updates logic to fit latest signatureCOntroller

* updates mmi extension package

* updates signature-controller and message-manager packages

* checkout develop lock file and run yarn

* checkout develop lock file and package.json to test circleci

* test fix

* adds signature-controller new version

* updates mmi extension package

* tx-list update and runs lavamoat auto

* lint fix

* runs lavamoat auto

* resets lavamoat/build-system/policy.jsono to develop

* Update LavaMoat policies

* adds back the dispatch

* lint

* changes needed to generate a mmi build

* adds metametricsId in url param

* adds necessary fence

---------

Co-authored-by: MetaMask Bot <[email protected]>
@pritam1813 pritam1813 requested a review from kumavis as a code owner July 25, 2023 10:57
Copy link
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.

LGTM! Thanks for your contribution

  • Pulled branch checked storybook locally ✅

@garrettbear garrettbear merged commit a942459 into MetaMask:develop Jul 25, 2023
9 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Jul 25, 2023
@pritam1813 pritam1813 deleted the part-of-#19528-import-token branch July 26, 2023 03:22
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
external-contributor team-design-system All issues relating to design system in Extension
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants