Part of #20163 for file ui\components\app\terms-of-use-popup\terms-of-use-popup.js by PrgrmrHarshShukla · Pull Request #20618 · MetaMask/metamask-extension

PrgrmrHarshShukla · 2023-08-26T16:41:36Z

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\terms-of-use-popup\terms-of-use-popup.js

Also fixed a small typo in
ui\components\app\snaps\install-error\install-error.stories.js

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.

…terms-of-use-popup.js

github-actions · 2023-08-26T16:41:47Z

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 · 2023-08-26T16:59:51Z

Hey @garrettbear and @georgewrmarshall ,

Kindly look at the positioning of the checkbox and label text.
There is some change in their positioning in the before and after UI.
I thought it would be better to ask if it should be made to follow the earlier positioning.

Also,
the label text was bold earlier.
I could not make out how to do so, please help me with some idea on how to do that, in case it is needed.

PrgrmrHarshShukla · 2023-08-26T17:02:59Z

Also,
please let me know if I should move the typo problem to a new PR to keep things separate.
I thought it is a relatively small problem, so it might be okay to keep it here.

Regards,
Harsh

georgewrmarshall

Hey @PrgrmrHarshShukla, these changes look good! Great call out on the visual regressions! This is an issue with the Checkbox component that can be addressed separately. There are some failing e2e tests that will need to be resolved for the PR to get merged. If you can kindly see to those that would be great!

PrgrmrHarshShukla · 2023-08-29T01:53:21Z

Sure @georgewrmarshall ,
Will definitely do that.

Regards

PrgrmrHarshShukla · 2023-08-30T00:59:29Z

Hey @georgewrmarshall ,
Both the e2e tests are failing because of the same reason.
The reason being TimeoutError while waiting for an element ( probably Checkbox element, we can get a hint here [data-testid="terms-of-use-checkbox"] below ) to be located by the CSS selector.

Also

while updating the snapshots for another PR #20535 , I encountered this,

Probably, these are connected in some way.

Possible Solution

Probably we can let the CSS selector grab the just higher level component of the Checkbox,
that is, the div with className="create-new-vault__terms".

Also,
here the Checkbox is rendered conditionally and the condition includeTerms is initially false, this might be making it unavailable for the CSS selector to grab.

🙏🏼
@georgewrmarshall , please share your insights on this.

Regards,
Harsh

…ents-app-terms-of-use-popup-terms-of-use-popup.js

georgewrmarshall

Left some suggestions

ui/components/app/terms-of-use-popup/terms-of-use-popup.js

Co-authored-by: George Marshall <georgewrmarshall@gmail.com>

…ents-app-terms-of-use-popup-terms-of-use-popup.js

georgewrmarshall

LGTM! Not sure why the e2e tests are failing here will try updating from develop

…ents-app-terms-of-use-popup-terms-of-use-popup.js

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

Hey @garrettbear ,
Your help is probably needed here.

…ents-app-terms-of-use-popup-terms-of-use-popup.js

codecov · 2024-01-17T01:48:59Z

Codecov Report

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

Comparison is base (1e5e386) 68.15% compared to head (0c811fe) 68.15%.

Files	Patch %	Lines
...nents/app/terms-of-use-popup/terms-of-use-popup.js	0.00%	1 Missing ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop   #20618      +/-   ##
===========================================
- Coverage    68.15%   68.15%   -0.01%     
===========================================
  Files         1083     1083              
  Lines        42491    42491              
  Branches     11333    11333              
===========================================
- Hits         28959    28956       -3     
- Misses       13532    13535       +3

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

…ents-app-terms-of-use-popup-terms-of-use-popup.js

georgewrmarshall

LGTM ✅

checked storybook

Part of MetaMask#20163 for file ui\components\app\terms-of-use-popup\…

e77945c

…terms-of-use-popup.js

PrgrmrHarshShukla requested review from a team as code owners August 26, 2023 16:41

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

georgewrmarshall reviewed Aug 28, 2023

View reviewed changes

PrgrmrHarshShukla added 2 commits September 8, 2023 06:02

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

1d4086d

…ents-app-terms-of-use-popup-terms-of-use-popup.js

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

e278bd2

…ents-app-terms-of-use-popup-terms-of-use-popup.js

georgewrmarshall requested changes Sep 26, 2023

View reviewed changes

ui/components/app/terms-of-use-popup/terms-of-use-popup.js Outdated Show resolved Hide resolved

ui/components/app/terms-of-use-popup/terms-of-use-popup.js Outdated Show resolved Hide resolved

PrgrmrHarshShukla and others added 7 commits September 26, 2023 20:32

Update ui/components/app/terms-of-use-popup/terms-of-use-popup.js

b30d42b

Co-authored-by: George Marshall <georgewrmarshall@gmail.com>

Update ui/components/app/terms-of-use-popup/terms-of-use-popup.js

69870f2

Co-authored-by: George Marshall <georgewrmarshall@gmail.com>

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

5c4622d

…ents-app-terms-of-use-popup-terms-of-use-popup.js

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

f23bc68

…ents-app-terms-of-use-popup-terms-of-use-popup.js

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

389ee28

…ents-app-terms-of-use-popup-terms-of-use-popup.js

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

a26eb69

…ents-app-terms-of-use-popup-terms-of-use-popup.js

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

5767f89

…ents-app-terms-of-use-popup-terms-of-use-popup.js

georgewrmarshall approved these changes Oct 12, 2023

View reviewed changes

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

1e2791f

…ents-app-terms-of-use-popup-terms-of-use-popup.js

georgewrmarshall added 2 commits January 12, 2024 19:30

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

74cf7a1

…ents-app-terms-of-use-popup-terms-of-use-popup.js

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

0c811fe

…ents-app-terms-of-use-popup-terms-of-use-popup.js

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

afe7e5a

…ents-app-terms-of-use-popup-terms-of-use-popup.js

garrettbear approved these changes Jan 24, 2024

View reviewed changes

georgewrmarshall approved these changes Jan 24, 2024

View reviewed changes

FrederikBolding approved these changes Jan 24, 2024

View reviewed changes

georgewrmarshall merged commit d212d3b into MetaMask:develop Jan 24, 2024

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Part of #20163 for file ui\components\app\terms-of-use-popup\terms-of-use-popup.js#20618

Part of #20163 for file ui\components\app\terms-of-use-popup\terms-of-use-popup.js#20618
georgewrmarshall merged 14 commits intoMetaMask:developfrom
PrgrmrHarshShukla:Part-of-#20163-for-file-ui-components-app-terms-of-use-popup-terms-of-use-popup.js

PrgrmrHarshShukla commented Aug 26, 2023

Uh oh!

github-actions bot commented Aug 26, 2023

Uh oh!

PrgrmrHarshShukla commented Aug 26, 2023

Uh oh!

PrgrmrHarshShukla commented Aug 26, 2023

Uh oh!

georgewrmarshall left a comment

Uh oh!

PrgrmrHarshShukla commented Aug 29, 2023

Uh oh!

PrgrmrHarshShukla commented Aug 30, 2023

Uh oh!

georgewrmarshall left a comment

Uh oh!

Uh oh!

Uh oh!

georgewrmarshall left a comment

Uh oh!

PrgrmrHarshShukla commented Nov 15, 2023

Uh oh!

codecov bot commented Jan 17, 2024

Uh oh!

georgewrmarshall left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

PrgrmrHarshShukla commented Aug 26, 2023

Explanation

Screenshots/Screencaps

Before

After

Manual Testing Steps

Pre-merge author checklist

Pre-merge reviewer checklist

Uh oh!

github-actions bot commented Aug 26, 2023

Uh oh!

PrgrmrHarshShukla commented Aug 26, 2023

Uh oh!

PrgrmrHarshShukla commented Aug 26, 2023

Uh oh!

georgewrmarshall left a comment

Choose a reason for hiding this comment

Uh oh!

PrgrmrHarshShukla commented Aug 29, 2023

Uh oh!

PrgrmrHarshShukla commented Aug 30, 2023

Also

Possible Solution

Uh oh!

georgewrmarshall left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

georgewrmarshall left a comment

Choose a reason for hiding this comment

Uh oh!

PrgrmrHarshShukla commented Nov 15, 2023

Uh oh!

codecov bot commented Jan 17, 2024

Codecov Report

Uh oh!

georgewrmarshall left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants