Part of #20163 for file ui\components\app\modals\eth-sign-modal\eth-sign-modal.js

[PrgrmrHarshShukla]

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]

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.

[PrgrmrHarshShukla]

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]

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]

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]

@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]

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

[codecov]

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]

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]

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

[georgewrmarshall]

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

[PrgrmrHarshShukla]

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

[PrgrmrHarshShukla]

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]

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 🙏

[georgewrmarshall]

LGTM! Thanks for your contribution @PrgrmrHarshShukla

[PrgrmrHarshShukla]

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

[garrettbear]

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

[georgewrmarshall]

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