Part of #20163 for file ui\components\app\modals\eth-sign-modal\eth-sign-modal.js by PrgrmrHarshShukla · Pull Request #20599 · MetaMask/metamask-extension

PrgrmrHarshShukla · 2023-08-25T01:55:12Z

Explanation

This PR is a part of the issue #20163 Replace deprecated CheckBox component with Checkbox from the component-library for file
ui\components\app\modals\eth-sign-modal\eth-sign-modal.js

Also updated the Box component with Box component from the component-library.

Screenshots/Screencaps

Before

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.

github-actions · 2023-08-25T01:55:25Z

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.

…ents-app-modals-eth-sign-modal-eth-sign-modal.js

PrgrmrHarshShukla · 2023-08-26T07:24:12Z

Hey @georgewrmarshall ,
If you get time, kindly give some ideas to help me tackle the problem stated in the comment ?

georgewrmarshall

Code looks good just need to update snapshots and I'll do a full review! Have commented in the thread you posted as well

PrgrmrHarshShukla · 2023-08-26T16:19:53Z

Hey @georgewrmarshall ,
As you asked in the comment,
I am using Windows.

Thank you for helping me out.
Will definitely ask @HowardBraham for help.

Regards,
Harsh

PrgrmrHarshShukla · 2023-08-26T16:30:37Z

Hello there @HowardBraham ,
If possible, kindly share some ideas on how I can solve the following problem.

I am using a Windows Operating System.

I am not able to update snapshots using the command yarn jest -u
I am getting these errors continuously.
1.)

2.)

Since JSDOM() is being taken into use on line 52 in test\helpers\setup-helper.js, I am not able to make out why the second error is there.

Kindly look at this comment for some more context.

Regards,
Harsh

HowardBraham · 2023-08-29T00:08:31Z

@PrgrmrHarshShukla This is a Node version problem. I bet you have Node v18.16.x. I just reproduced it myself.
This issue was okay in v18.15.0, broke in v18.16.0, fixed in v18.17.0: nodejs/node/issues/47563

PrgrmrHarshShukla · 2023-08-29T01:03:00Z

You are absolutely right.
I actually have node v18.16.1.
Will move to v18.15.0 or v18.17.0.

Thanks a lot for helping me out.

Regards,
Harsh

…ents-app-modals-eth-sign-modal-eth-sign-modal.js

codecov · 2023-09-08T01:00:15Z

Codecov Report

Attention: 1 lines in your changes are missing coverage. Please review.

Comparison is base (2026ef9) 68.61% compared to head (4f7c952) 68.86%.

❗ Current head 4f7c952 differs from pull request most recent head 3ab3963. Consider uploading reports for the commit 3ab3963 to get more accurate results

Additional details and impacted files

@@             Coverage Diff             @@
##           develop   #20599      +/-   ##
===========================================
+ Coverage    68.61%   68.86%   +0.26%     
===========================================
  Files         1017      998      -19     
  Lines        40814    38949    -1865     
  Branches     10900    10468     -432     
===========================================
- Hits         28002    26822    -1180     
+ Misses       12812    12127     -685

Files	Coverage Δ
...onents/app/modals/eth-sign-modal/eth-sign-modal.js	`45.16% <0.00%> (ø)`

... and 192 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

PrgrmrHarshShukla · 2023-09-10T10:07:54Z

Hey @georgewrmarshall ,
All the tests have passed for this PR.
Kindly look into this so that any remaining problems can be solved.

Regards,
Harsh

PrgrmrHarshShukla · 2023-09-18T11:05:14Z

Hey @georgewrmarshall , All the tests have passed for this PR. Kindly look into this so that any remaining problems can be solved.

Regards, Harsh

Hey @georgewrmarshall , @garrettbear ,
Please look into this.

Regards,
Harsh

…n eth-sign-modal component

georgewrmarshall

LGTM! Pushed some small fixes to attributes and props while I had the branch pulled. Thanks for your contribution @PrgrmrHarshShukla!

PrgrmrHarshShukla · 2023-09-26T15:07:03Z

Hey @georgewrmarshall ,
Probably we need to update the snapshots.
Shall I add a commit for that?

PrgrmrHarshShukla · 2023-09-26T15:20:28Z

Thanks a lot for doing all these changes to these PRs yourself. 🙏🏼
I am learning a lot through them and working hard to bring better PRs in the future.
I am trying my best to open PRs for only those issues I can solve really well and with minimum or no errors at all unlike the previous ones.
I hope, this way, I will improve with time and do not waste the precious time of reviewers.

georgewrmarshall · 2023-10-02T12:23:17Z

Hey @PrgrmrHarshShukla, no problem! I appreciate you taking on these tasks and helping us reduce tech debt and improve the UI consistency of the extension! And yes! Any snapshot test failures are mistakes on my side please go ahead and update whenever you see failing tests 🙏

…ents-app-modals-eth-sign-modal-eth-sign-modal.js

georgewrmarshall

LGTM! Thanks for your contribution @PrgrmrHarshShukla

…ents-app-modals-eth-sign-modal-eth-sign-modal.js

PrgrmrHarshShukla · 2023-11-15T10:32:32Z

Hey @garrettbear ,
Your help is probably needed here too.

…ents-app-modals-eth-sign-modal-eth-sign-modal.js

garrettbear · 2024-01-24T04:34:19Z

ui/components/app/modals/eth-sign-modal/eth-sign-modal.js

-          <CheckBox
-            id="eth-sign__checkbox"
+          <Checkbox
+            id="eth-sign-checkbox"


All looks good, was there any reason for changing this id @georgewrmarshall ? I see it has no impact

No reason I think this was a preconceived notion that we shouldn't use underscores in id names. Looks doesn't look like it's used anywhere other than here so it's safe to update

Update eth-sign-modal.js

e399a12

PrgrmrHarshShukla requested a review from a team as a code owner August 25, 2023 01:55

Merge branch 'develop' into Part-of-MetaMask#20163-for-file-ui-compon…

4c0b49b

…ents-app-modals-eth-sign-modal-eth-sign-modal.js

PrgrmrHarshShukla changed the title ~~Part of @20163 for file ui\components\app\modals\eth-sign-modal\eth-sign-modal.js~~ Part of #20163 for file ui\components\app\modals\eth-sign-modal\eth-sign-modal.js Aug 25, 2023

georgewrmarshall added the team-design-system All issues relating to design system in Extension label Aug 26, 2023

georgewrmarshall reviewed Aug 26, 2023

View reviewed changes

PrgrmrHarshShukla and others added 3 commits August 29, 2023 06:56

Update eth-sign-modal.test.js.snap

5212141

Merge branch 'develop' into Part-of-MetaMask#20163-for-file-ui-compon…

210a488

…ents-app-modals-eth-sign-modal-eth-sign-modal.js

Merge branch 'develop' into Part-of-MetaMask#20163-for-file-ui-compon…

c100fdc

…ents-app-modals-eth-sign-modal-eth-sign-modal.js

Fixing attributes and casing conventions for the checkbox component i…

547d99d

…n eth-sign-modal component

georgewrmarshall previously approved these changes Sep 26, 2023

View reviewed changes

Update eth-sign-modal.test.js.snap

4f7c952

PrgrmrHarshShukla dismissed georgewrmarshall’s stale review via 4f7c952 October 3, 2023 01:28

Merge branch 'develop' into Part-of-MetaMask#20163-for-file-ui-compon…

3ab3963

…ents-app-modals-eth-sign-modal-eth-sign-modal.js

georgewrmarshall previously approved these changes Oct 10, 2023

View reviewed changes

Merge branch 'develop' into Part-of-MetaMask#20163-for-file-ui-compon…

ffc61d4

…ents-app-modals-eth-sign-modal-eth-sign-modal.js

Fixing merge conflict in eth-sign-modal

8863153

georgewrmarshall dismissed their stale review via 8863153 January 12, 2024 23:15

georgewrmarshall added 2 commits January 16, 2024 17:27

Merge branch 'develop' into Part-of-MetaMask#20163-for-file-ui-compon…

51a8871

…ents-app-modals-eth-sign-modal-eth-sign-modal.js

Merge branch 'develop' into Part-of-MetaMask#20163-for-file-ui-compon…

ab73aa0

…ents-app-modals-eth-sign-modal-eth-sign-modal.js

garrettbear reviewed Jan 24, 2024

View reviewed changes

garrettbear approved these changes Jan 24, 2024

View reviewed changes

georgewrmarshall approved these changes Jan 24, 2024

View reviewed changes

georgewrmarshall merged commit 978e57a into MetaMask:develop Jan 24, 2024

github-actions bot locked and limited conversation to collaborators Jan 24, 2024

Conversation

PrgrmrHarshShukla commented Aug 25, 2023

Explanation

Screenshots/Screencaps

Before

After

Manual Testing Steps

Pre-merge author checklist

Pre-merge reviewer checklist

Uh oh!

github-actions bot commented Aug 25, 2023

Uh oh!

PrgrmrHarshShukla commented Aug 26, 2023

Uh oh!

georgewrmarshall left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

PrgrmrHarshShukla commented Aug 26, 2023

Uh oh!

PrgrmrHarshShukla commented Aug 26, 2023

Uh oh!

HowardBraham commented Aug 29, 2023

Uh oh!

PrgrmrHarshShukla commented Aug 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Sep 8, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

PrgrmrHarshShukla commented Sep 10, 2023

Uh oh!

PrgrmrHarshShukla commented Sep 18, 2023

Uh oh!

georgewrmarshall left a comment

Choose a reason for hiding this comment

Uh oh!

PrgrmrHarshShukla commented Sep 26, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

PrgrmrHarshShukla commented Sep 26, 2023

Uh oh!

georgewrmarshall commented Oct 2, 2023

Uh oh!

georgewrmarshall left a comment

Choose a reason for hiding this comment

Uh oh!

PrgrmrHarshShukla commented Nov 15, 2023

Uh oh!

garrettbear Jan 24, 2024

Choose a reason for hiding this comment

Uh oh!

georgewrmarshall Jan 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

georgewrmarshall left a comment •

edited

Loading

PrgrmrHarshShukla commented Aug 29, 2023 •

edited

Loading

codecov bot commented Sep 8, 2023 •

edited

Loading

PrgrmrHarshShukla commented Sep 26, 2023 •

edited

Loading

georgewrmarshall Jan 24, 2024 •

edited

Loading