fix/BannerBase to TS #20421

dhruvv173 · 2023-08-07T14:20:04Z

Explanation

Fixes Migrate components to TS: BannerBase #19181

Screenshots/Screencaps

Before

before.mp4

After

after.mp4

Manual Testing Steps

Run yarn storybook on this branch
Check controls for BannerBase
yarn jest ui/components/ocmponent-library/banner-base

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-07T14:20:22Z

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.

ui/components/component-library/banner-base/banner-base.types.ts

Ameralameri

Approved

codecov · 2023-08-08T03:15:56Z

Codecov Report

Merging #20421 (9c8e160) into develop (88212a7) will decrease coverage by 0.00%.
Report is 11 commits behind head on develop.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop   #20421      +/-   ##
===========================================
- Coverage    68.68%   68.68%   -0.00%     
===========================================
  Files          991      991              
  Lines        38451    38452       +1     
  Branches     10329    10330       +1     
===========================================
- Hits         26409    26408       -1     
- Misses       12042    12044       +2

Files Changed	Coverage Δ
...pages/swaps/prepare-swap-page/prepare-swap-page.js	`69.35% <ø> (ø)`
ui/pages/swaps/prepare-swap-page/review-quote.js	`68.97% <ø> (ø)`
...s/prepare-swap-page/view-quote-price-difference.js	`100.00% <ø> (ø)`
...ges/swaps/swaps-banner-alert/swaps-banner-alert.js	`87.27% <ø> (ø)`
...swaps/transaction-settings/transaction-settings.js	`79.83% <ø> (ø)`
...ents/component-library/banner-base/banner-base.tsx	`100.00% <100.00%> (ø)`

... and 2 files with indirect coverage changes

ui/components/component-library/banner-base/banner-base.types.ts

georgewrmarshall · 2023-08-15T03:07:46Z

ui/components/component-library/banner-base/banner-base.types.ts

+type MakePropsOptional<T> = {
+  [K in keyof T]?: T[K];
+};


This type helper allows for us to use prop objects without needing to pass all required props through the prop object. @brad-decker do you have any suggestions on where a type like this should live? Should we create a types.ts file in the root of the ui/components/component-library/types.ts or maybe in shared/constants/component-library.ts?

typescript.after.mov

cc @garrettbear

georgewrmarshall · 2023-08-15T03:09:56Z

ui/components/component-library/box/box.types.ts

+   * TypeScript is complaining about data- attributes which means we need to explicitly define this as a prop.
+   * TODO: Allow data- attributes.
+   */
+  'data-testid'?: string;


Temporary fix to allow data-testids being passed to prop objects which is generally what they will be used for

georgewrmarshall

LGTM! I made some small updates based on your suggestions while I had your branch checked out. Thanks for your contribution @dhruvv173

georgewrmarshall

Hey @dhruvv173, this PR should fix the linting error #20448 and looks like I missed some snapshots or they were updated in the last merge from develop. I was also thinking that because I added the childrenWrapperProps we should write a unit test for that. There should be some examples of this with titleProps in the unit tests. Would you mind updating it?

dhruvv173 · 2023-08-15T15:49:39Z

Hey @dhruvv173, this PR should fix the linting error #20448 and looks like I missed some snapshots or they were updated in the last merge from develop. I was also thinking that because I added the childrenWrapperProps we should write a unit test for that. There should be some examples of this with titleProps in the unit tests. Would you mind updating it?

I have updated the snapshot locally and lint error will be fixed once your PR is merged. About the latter part, I just want to confirm that we want to add a unit test which checks for childrenWrapperProps in the component, right? If yes, I will check for examples in unit tests and work on it. Thank you!

georgewrmarshall · 2023-08-15T20:09:50Z

Hey @dhruvv173, this PR should fix the linting error #20448 and looks like I missed some snapshots or they were updated in the last merge from develop. I was also thinking that because I added the childrenWrapperProps we should write a unit test for that. There should be some examples of this with titleProps in the unit tests. Would you mind updating it?

I have updated the snapshot locally and lint error will be fixed once your PR is merged. About the latter part, I just want to confirm that we want to add a unit test which checks for childrenWrapperProps in the component, right? If yes, I will check for examples in unit tests and work on it. Thank you!

Yes, that's correct! I noticed that we aren't actually testing for titleProps or descriptionProps we can improve the coverage by adjusting each unit test for these props slightly

it('should render bannerbase title', () => {
  const { getByText, getByTestId } = render(
    <BannerBase
      title="Bannerbase title test"
      titleProps={{ 'data-testid': 'title' }}
    />,
  );
 
  expect(getByText('Bannerbase title test')).toHaveClass('mm-banner-base__title');
  expect(getByTestId('title')).toBeDefined();
});;

Changing this element to a tag: 'p' instead of h5 tag should also help with the e2e tests

metamask-extension/test/e2e/tests/security-provider.spec.js

Line 109 in b0fcb12

tag: 'h5',

dhruvv173 · 2023-08-16T03:34:11Z

hi @georgewrmarshall, what do think about something like this?

  it('should render bannerbase children with props', () => {
    const { getByText, getByTestId } = render(
      <BannerBase
        childrenWrapperProps={{
          variant: TextVariant.bodyMd,
          'data-testid': 'childrenWrapper',
        }}
      >
        Bannerbase children test
      </BannerBase>,
    );
    expect(getByTestId('childrenWrapper')).toBeDefined();
    expect(getByText('Bannerbase children test')).toBeDefined();
  });

georgewrmarshall · 2023-08-16T17:58:15Z

hi @georgewrmarshall, what do think about something like this?

  it('should render bannerbase children with props', () => {
    const { getByText, getByTestId } = render(
      <BannerBase
        childrenWrapperProps={{
          variant: TextVariant.bodyMd,
          'data-testid': 'childrenWrapper',
        }}
      >
        Bannerbase children test
      </BannerBase>,
    );
    expect(getByTestId('childrenWrapper')).toBeDefined();
    expect(getByText('Bannerbase children test')).toBeDefined();
  });

Looks good!

dhruvv173 · 2023-08-16T18:46:48Z

Hey @dhruvv173, this PR should fix the linting error #20448 and looks like I missed some snapshots or they were updated in the last merge from develop. I was also thinking that because I added the childrenWrapperProps we should write a unit test for that. There should be some examples of this with titleProps in the unit tests. Would you mind updating it?

I have updated the snapshot locally and lint error will be fixed once your PR is merged. About the latter part, I just want to confirm that we want to add a unit test which checks for childrenWrapperProps in the component, right? If yes, I will check for examples in unit tests and work on it. Thank you!

Yes, that's correct! I noticed that we aren't actually testing for titleProps or descriptionProps we can improve the coverage by adjusting each unit test for these props slightly
it('should render bannerbase title', () => {
  const { getByText, getByTestId } = render(
    <BannerBase
      title="Bannerbase title test"
      titleProps={{ 'data-testid': 'title' }}
    />,
  );
 
  expect(getByText('Bannerbase title test')).toHaveClass('mm-banner-base__title');
  expect(getByTestId('title')).toBeDefined();
});;
Changing this element to a tag: 'p' instead of h5 tag should also help with the e2e tests

metamask-extension/test/e2e/tests/security-provider.spec.js

Line 109 in b0fcb12

tag: 'h5',

every instance of h5 should be replaced with p, right?

…hildrenWrapperProps

dhruvv173 · 2023-08-16T18:51:33Z

Looks good!

added here please check when possible, thank you!

…ng static data-testid in favor of using it at the instance, updating snapshots associated with those changes

georgewrmarshall · 2023-08-17T21:15:22Z

test/e2e/swaps/shared.js

@@ -157,7 +157,7 @@ const checkActivityTransaction = async (driver, options) => {

 const checkNotification = async (driver, options) => {
  const boxTitle = await driver.findElement(
-    '[data-testid="mm-banner-base-title"]',
+    '[data-testid="swap-token-verification-banner-title"]',


Removing the static data-testid that was added to the component in favor of using descriptionProps and making the test id more specific"

georgewrmarshall · 2023-08-17T21:16:57Z

ui/components/component-library/banner-base/banner-base.tsx

+          {children && typeof children === 'object' ? (
+            children
+          ) : (
+            <Text {...childrenWrapperProps}>{children}</Text>


adds childrenWrapperProps here

georgewrmarshall · 2023-08-17T21:17:44Z

ui/components/component-library/banner-base/banner-base.tsx

+
+        <div>
+          {title && (
+            <Text variant={TextVariant.bodyLgMedium} {...titleProps}>


Removes static data-testid from title to enable specificity of test id during implementation

georgewrmarshall · 2023-08-17T21:18:49Z

ui/pages/swaps/prepare-swap-page/prepare-swap-page.js

@@ -1036,6 +1036,9 @@ export default function PrepareSwapPage({
                  ? t('swapTokenVerifiedOn1SourceTitle')
                  : t('swapTokenAddedManuallyTitle')
              }
+              titleProps={{


Here we are removing the static data-testid from the BannerBase component and utilizing titleProps to assign one at implementation

georgewrmarshall · 2023-08-17T21:21:14Z

ui/components/component-library/banner-base/banner-base.stories.tsx

    children: {
      control: 'text',
    },
+    childrenProps: {
+      control: 'object',
+    },
    action: {


This shouldn't be here remove

georgewrmarshall

LGTM! I made some small updates to tests and updated the component to remove the static test id and a class name that I don't think we needed. Just need tests to pass. Thanks for your contribution @dhruvv173

garrettbear

Nice! LGTM. Just looks like we need to sort out some e2e tests that are failing

…se. Also updating snapshot of ConfirmTransaction page

BannerBase to TS

4495eab

dhruvv173 requested a review from a team as a code owner August 7, 2023 14:20

dhruvv173 commented Aug 7, 2023

View reviewed changes

ui/components/component-library/banner-base/banner-base.types.ts Outdated Show resolved Hide resolved

Ameralameri approved these changes Aug 7, 2023

View reviewed changes

dhruvv173 added 4 commits August 7, 2023 23:16

snapshot updates

5a56d40

more snapshot updates

d2ff9fe

addressing type definition error

ed4295e

updating eth-sign-modal snapshot

160fb7d

dhruvv173 commented Aug 8, 2023

View reviewed changes

ui/components/component-library/banner-base/banner-base.types.ts Outdated Show resolved Hide resolved

dhruvv173 commented Aug 8, 2023

View reviewed changes

ui/components/component-library/banner-base/banner-base.types.ts Outdated Show resolved Hide resolved

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

georgewrmarshall added 2 commits August 14, 2023 19:47

Updates to stories, types and adding data-testid

9c5e214

Updating snapshots

ad6e53b

georgewrmarshall reviewed Aug 15, 2023

View reviewed changes

georgewrmarshall approved these changes Aug 15, 2023

View reviewed changes

Merge branch 'develop' into fix/19181

1a070e2

georgewrmarshall requested changes Aug 15, 2023

View reviewed changes

Merge branch 'develop' into fix/19181

f6dcab1

dhruvv173 mentioned this pull request Aug 16, 2023

Fix: Migrate TextField to TS #20477

Merged

8 tasks

updating snapshot of blockaid-banner-alert and adding unit test for c…

c30aa29

…hildrenWrapperProps

BannerBase updates to stories, adding locale for close button, removi…

e2ea36d

…ng static data-testid in favor of using it at the instance, updating snapshots associated with those changes

georgewrmarshall reviewed Aug 17, 2023

View reviewed changes

Merge branch 'develop' into fix/19181

c6d9be5

georgewrmarshall reviewed Aug 17, 2023

View reviewed changes

Removing incorrect arg from storybook file

5a606de

georgewrmarshall previously approved these changes Aug 17, 2023

View reviewed changes

georgewrmarshall requested a review from garrettbear August 17, 2023 21:24

garrettbear reviewed Aug 17, 2023

View reviewed changes

Updating html element in security provider e2e test to match BannerBa…

35aa4d8

…se. Also updating snapshot of ConfirmTransaction page

georgewrmarshall dismissed their stale review via 35aa4d8 August 17, 2023 22:48

georgewrmarshall previously approved these changes Aug 17, 2023

View reviewed changes

georgewrmarshall requested a review from garrettbear August 17, 2023 22:49

garrettbear previously approved these changes Aug 17, 2023

View reviewed changes

Fixing e2e tests

9c8e160

georgewrmarshall dismissed stale reviews from garrettbear and themself via 9c8e160 August 18, 2023 20:53

georgewrmarshall requested a review from dan437 August 18, 2023 20:53

georgewrmarshall approved these changes Aug 18, 2023

View reviewed changes

georgewrmarshall requested a review from garrettbear August 18, 2023 20:55

garrettbear approved these changes Aug 18, 2023

View reviewed changes

garrettbear merged commit 07abc53 into MetaMask:develop Aug 18, 2023
9 checks passed

github-actions bot locked and limited conversation to collaborators Aug 18, 2023

dhruvv173 deleted the fix/19181 branch August 19, 2023 05:38

georgewrmarshall added the external-contributor label Oct 10, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix/BannerBase to TS #20421

fix/BannerBase to TS #20421

dhruvv173 commented Aug 7, 2023

github-actions bot commented Aug 7, 2023

Ameralameri left a comment

codecov bot commented Aug 8, 2023 •

edited

Loading

georgewrmarshall Aug 15, 2023 •

edited

Loading

georgewrmarshall Aug 15, 2023

georgewrmarshall Aug 15, 2023

georgewrmarshall left a comment

georgewrmarshall left a comment

dhruvv173 commented Aug 15, 2023

georgewrmarshall commented Aug 15, 2023 •

edited

Loading

dhruvv173 commented Aug 16, 2023

georgewrmarshall commented Aug 16, 2023

dhruvv173 commented Aug 16, 2023

dhruvv173 commented Aug 16, 2023

georgewrmarshall Aug 17, 2023

georgewrmarshall Aug 17, 2023

georgewrmarshall Aug 17, 2023

georgewrmarshall Aug 17, 2023

georgewrmarshall Aug 17, 2023

georgewrmarshall left a comment

garrettbear left a comment •

edited

Loading

fix/BannerBase to TS #20421

fix/BannerBase to TS #20421

Conversation

dhruvv173 commented Aug 7, 2023

Explanation

Screenshots/Screencaps

Before

After

Manual Testing Steps

Pre-merge author checklist

Pre-merge reviewer checklist

github-actions bot commented Aug 7, 2023

Ameralameri left a comment

Choose a reason for hiding this comment

codecov bot commented Aug 8, 2023 • edited Loading

Codecov Report

georgewrmarshall Aug 15, 2023 • edited Loading

Choose a reason for hiding this comment

georgewrmarshall Aug 15, 2023

Choose a reason for hiding this comment

georgewrmarshall Aug 15, 2023

Choose a reason for hiding this comment

georgewrmarshall left a comment

Choose a reason for hiding this comment

georgewrmarshall left a comment

Choose a reason for hiding this comment

dhruvv173 commented Aug 15, 2023

georgewrmarshall commented Aug 15, 2023 • edited Loading

dhruvv173 commented Aug 16, 2023

georgewrmarshall commented Aug 16, 2023

dhruvv173 commented Aug 16, 2023

dhruvv173 commented Aug 16, 2023

georgewrmarshall Aug 17, 2023

Choose a reason for hiding this comment

georgewrmarshall Aug 17, 2023

Choose a reason for hiding this comment

georgewrmarshall Aug 17, 2023

Choose a reason for hiding this comment

georgewrmarshall Aug 17, 2023

Choose a reason for hiding this comment

georgewrmarshall Aug 17, 2023

Choose a reason for hiding this comment

georgewrmarshall left a comment

Choose a reason for hiding this comment

garrettbear left a comment • edited Loading

Choose a reason for hiding this comment

codecov bot commented Aug 8, 2023 •

edited

Loading

georgewrmarshall Aug 15, 2023 •

edited

Loading

georgewrmarshall commented Aug 15, 2023 •

edited

Loading

garrettbear left a comment •

edited

Loading