Part of #17670 for file ui\pages\onboarding-flow\create-password\create-password.js by PrgrmrHarshShukla · Pull Request #20795 · MetaMask/metamask-extension

PrgrmrHarshShukla · 2023-09-08T03:06:31Z

Explanation

This PR is a part of the issue #17670 Replace deprecated Typography with Text component for file
ui\pages\onboarding-flow\create-password\create-password.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.

…ord\create-password.js

github-actions · 2023-09-08T03:06:45Z

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-09-08T06:51:56Z

Hey @georgewrmarshall , @garrettbear ,
Kindly look into this PR once you get time.

Some inconsistencies

I had to use bodyXs for H7 instead of bodyMd to better match the current UI.
Similar changes had to be made for H2 and H4 to better match with the current UI.
Please let me know if that is all right or not.

Visual regression

Presently, I can see a small visual regression in the alignment of New Password and Show/Hide toggle.

If we add an extra marginBottom={1} it gets solved,

Also,

I am working on replacing the deprecated components, like the Button, CheckBox, etc.

Will introduce those changes in next commit in this PR if you find it OK.

codecov · 2023-09-08T07:12:27Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (1e5e386) 68.15% compared to head (540f233) 68.15%.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop   #20795      +/-   ##
===========================================
- Coverage    68.15%   68.15%   -0.00%     
===========================================
  Files         1083     1083              
  Lines        42491    42491              
  Branches     11333    11333              
===========================================
- Hits         28959    28957       -2     
- Misses       13532    13534       +2

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

PrgrmrHarshShukla · 2023-09-09T13:38:08Z

Hey @georgewrmarshall , @garrettbear , Kindly look into this PR once you get time.

Some inconsistencies

I had to use bodyXs for H7 instead of bodyMd to better match the current UI. Similar changes had to be made for H2 and H4 to better match with the current UI. Please let me know if that is all right or not.

Visual regression

Presently, I can see a small visual regression in the alignment of New Password and Show/Hide toggle.

If we add an extra marginBottom={1} it gets solved,

Also,

I am working on replacing the deprecated components, like the Button, CheckBox, etc.

Will introduce those changes in next commit in this PR if you find it OK.

Hey @georgewrmarshall ,
Would you like to say something about this?

Regards,
Harsh

PrgrmrHarshShukla · 2023-09-12T15:15:45Z

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

Regards,
Harsh

PrgrmrHarshShukla · 2023-09-17T13:02:06Z

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

Regards, Harsh

Hey @georgewrmarshall ,
Please look into this.

Regards,
Harsh

georgewrmarshall

Looking good @PrgrmrHarshShukla! Left some small suggestions. Could we also create a storybook file for this component

import React from 'react';
import CreatePassword from './create-password';

export default {
  title: 'Pages/OnboardingFlow/CreatePassword',
  argTypes: {
    createNewAccount: {
      action: 'createNewAccount',
    },
    importWithRecoveryPhrase: {
      action: 'importWithRecoveryPhrase',
    },
    secretRecoveryPhrase: {
      control: 'text',
    },
  },
};

export const DefaultStory = (args) => <CreatePassword {...args} />;

DefaultStory.storyName = 'Default';

ui/pages/onboarding-flow/create-password/create-password.js

georgewrmarshall · 2023-09-25T19:05:19Z

ui/pages/onboarding-flow/create-password/create-password.js

            value={password}
            titleDetail={
-              <Typography variant={TypographyVariant.H7}>
+              <Text variant={TextVariant.bodyXs}>


Let's replace the Text and link element with the ButtonLink component. Then we will be reducing the html bloat and using the correct semantic html element

<ButtonLink variant={TextVariant.bodySm} data-testid="show-password" className="create-password__form--password-button" onClick={(e) => { e.preventDefault(); setShowPassword(!showPassword); }} marginBottom={1} > {showPassword ? t('hide') : t('show')} </ButtonLink>

Sure,
Will do it now.

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

PrgrmrHarshShukla · 2023-09-26T01:27:57Z

Looking good @PrgrmrHarshShukla! Left some small suggestions. Could we also create a storybook file for this component

import React from 'react';
import CreatePassword from './create-password';

export default {
  title: 'Pages/OnboardingFlow/CreatePassword',
  argTypes: {
    createNewAccount: {
      action: 'createNewAccount',
    },
    importWithRecoveryPhrase: {
      action: 'importWithRecoveryPhrase',
    },
    secretRecoveryPhrase: {
      control: 'text',
    },
  },
};

export const DefaultStory = (args) => <CreatePassword {...args} />;

DefaultStory.storyName = 'Default';

Sure,
I will try and do that as soon as possible.

PrgrmrHarshShukla · 2023-09-26T01:52:42Z

Looking good @PrgrmrHarshShukla! Left some small suggestions. Could we also create a storybook file for this component

Hey @georgewrmarshall ,
You have provided me the complete code.
Thanks a lot.🙏🏼
I had to just add it there like the others.

Done.
🙏🏼

PrgrmrHarshShukla · 2023-09-26T01:57:45Z

Hey @georgewrmarshall , You have provided me the complete code. Thanks a lot.🙏🏼 I had to just add it there like the others.

Done. 🙏🏼

Storybook file added for the component.

PrgrmrHarshShukla · 2023-09-26T03:09:47Z

Hey @georgewrmarshall ,
All checks have passed here.

Please look into it for any remaining improvements.

georgewrmarshall

LGTM! Thanks for your contribution @PrgrmrHarshShukla

ui/pages/onboarding-flow/create-password/create-password.js

garrettbear · 2023-10-05T16:25:56Z

ui/pages/onboarding-flow/create-password/__snapshots__/create-password.test.js.snap

    </h2>
    <h4
-      class="box box--margin-top-1 box--margin-bottom-1 box--flex-direction-row typography typography--h4 typography--weight-normal typography--style-normal typography--align-center typography--color-text-default"
+      align="center"


When updating the prop from the below suggestion it should remove this, so you will need to update snapshot

Sure,
Will do that now.

Co-authored-by: Garrett Bear <gwhisten@gmail.com>

PrgrmrHarshShukla · 2023-10-05T17:25:09Z

Hey @garrettbear ,
Please help me with how to resolve the above conflict in the file
ui/pages/onboarding-flow/create-password/__snapshots__/create-password.test.js.snap

PrgrmrHarshShukla · 2023-10-06T10:16:34Z

Hey @garrettbear ,
Tried to solve the merge conflicts.
Please check the files to see everything is fine.

georgewrmarshall

Hey @PrgrmrHarshShukla, looks like there is some unintentional code that has been removed in a rebase. We want to keep all the logic just replace stale components. Would you be able to revert those changes so it's just replacement of the components?

ui/pages/onboarding-flow/create-password/create-password.js

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

georgewrmarshall

I pushed a fix for the missing closing environment tag. Let's see if there are any tests we need to update

georgewrmarshall

LGTM!

pulled branch checked storybook
pull branch checked local build

after.mov

PrgrmrHarshShukla · 2023-11-15T10:31:56Z

Hey @garrettbear ,
Your help is probably needed here.

Part of MetaMask#17670 for file ui\pages\onboarding-flow\create-passw…

c1606b4

…ord\create-password.js

PrgrmrHarshShukla requested a review from a team as a code owner September 8, 2023 03:06

PrgrmrHarshShukla added 2 commits September 8, 2023 11:50

Update create-password.js

1217489

Update create-password.test.js.snap

44ed6b7

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

georgewrmarshall requested changes Sep 25, 2023

View reviewed changes

Update ui/pages/onboarding-flow/create-password/create-password.js

0eaac8d

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

PrgrmrHarshShukla added 2 commits September 26, 2023 07:05

Replace the Text and Link element with the ButtonLink component

3f32d04

Create create-password.stories.js

53d1b78

Update create-password.test.js.snap

f522fe2

georgewrmarshall previously approved these changes Sep 26, 2023

View reviewed changes

garrettbear reviewed Oct 5, 2023

View reviewed changes

ui/pages/onboarding-flow/create-password/create-password.js Outdated Show resolved Hide resolved

garrettbear suggested changes Oct 5, 2023

View reviewed changes

Update ui/pages/onboarding-flow/create-password/create-password.js

804fa0c

Co-authored-by: Garrett Bear <gwhisten@gmail.com>

PrgrmrHarshShukla dismissed georgewrmarshall’s stale review via 804fa0c October 5, 2023 17:00

Update create-password.test.js.snap

0e1740a

Merge branch 'develop' into New-Experiment

66c73cc

georgewrmarshall requested changes Oct 7, 2023

View reviewed changes

PrgrmrHarshShukla and others added 5 commits October 7, 2023 23:53

Update ui/pages/onboarding-flow/create-password/create-password.js

fd2b66c

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

Update ui/pages/onboarding-flow/create-password/create-password.js

80ec655

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

Update ui/pages/onboarding-flow/create-password/create-password.js

b8ee820

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

Merge branch 'develop' into New-Experiment

2eabdcf

Adding closing environment comment tag to create-password

51c7a74

georgewrmarshall reviewed Oct 12, 2023

View reviewed changes

PrgrmrHarshShukla and others added 3 commits October 12, 2023 23:59

Update create-password.test.js.snap

6e797fb

Merge branch 'develop' into New-Experiment

2f00ca1

Merge branch 'develop' into New-Experiment

ea2aea7

georgewrmarshall approved these changes Oct 26, 2023

View reviewed changes

georgewrmarshall requested a review from garrettbear January 12, 2024 23:26

georgewrmarshall added 3 commits January 12, 2024 19:28

Merge branch 'develop' into New-Experiment

d1e3cb4

Merge branch 'develop' into New-Experiment

540f233

Merge branch 'develop' into New-Experiment

628a8be

garrettbear approved these changes Jan 24, 2024

View reviewed changes

garrettbear merged commit 39a0405 into MetaMask:develop Jan 24, 2024

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

Conversation

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

Uh oh!

Explanation

Screenshots/Screencaps

Before

After

Manual Testing Steps

Pre-merge author checklist

Pre-merge reviewer checklist

Uh oh!

github-actions bot commented Sep 8, 2023

Uh oh!

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

Uh oh!

Some inconsistencies

Visual regression

Also,

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 9, 2023

Some inconsistencies

Visual regression

Also,

Uh oh!

PrgrmrHarshShukla commented Sep 12, 2023

Uh oh!

PrgrmrHarshShukla commented Sep 17, 2023

Uh oh!

georgewrmarshall left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

georgewrmarshall Sep 25, 2023

Choose a reason for hiding this comment

Uh oh!

PrgrmrHarshShukla Sep 26, 2023

Choose a reason for hiding this comment

Uh oh!

PrgrmrHarshShukla commented Sep 26, 2023

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 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Storybook file added for the component.

Uh oh!

PrgrmrHarshShukla commented Sep 26, 2023

Uh oh!

georgewrmarshall left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

garrettbear Oct 5, 2023

Choose a reason for hiding this comment

Uh oh!

PrgrmrHarshShukla Oct 5, 2023

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Uh oh!

PrgrmrHarshShukla commented Oct 6, 2023

Uh oh!

georgewrmarshall left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

georgewrmarshall left a comment

Choose a reason for hiding this comment

Uh oh!

georgewrmarshall left a comment

Choose a reason for hiding this comment

PrgrmrHarshShukla commented Sep 8, 2023 •

edited

Loading

PrgrmrHarshShukla commented Sep 8, 2023 •

edited

Loading

codecov bot commented Sep 8, 2023 •

edited

Loading

PrgrmrHarshShukla commented Sep 26, 2023 •

edited

Loading

PrgrmrHarshShukla commented Sep 26, 2023 •

edited

Loading

PrgrmrHarshShukla commented Oct 5, 2023 •

edited

Loading