Skip to content

Fixing Broken TokenAllowance Story#20422

Merged
georgewrmarshall merged 26 commits intoMetaMask:developfrom
Jainex17:fix/17343/broken-story-TokenAllowance
Jan 23, 2024
Merged

Fixing Broken TokenAllowance Story#20422
georgewrmarshall merged 26 commits intoMetaMask:developfrom
Jainex17:fix/17343/broken-story-TokenAllowance

Conversation

@Jainex17
Copy link
Copy Markdown
Contributor

@Jainex17 Jainex17 commented Aug 8, 2023

Explanation

The TokenAllowance story was not being displayed in the Storybook. This PR resolves the issue and now it is shown in the Storybook.

chnages made

  • Renamed token-allowance.stories-to-do.js to token-allowance.stories.js.
  • Fixed issues in token-allowance.stories.js.
  • Added AllBannerAlert story to display all alerts

Screenshots/Screencaps

Before

before.mov

After

after.mov

Manual Testing Steps

  • Pull this branch
  • Run storybook
  • Search TokenAllowance

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

@Jainex17 Jainex17 requested a review from a team as a code owner August 8, 2023 07:25
@github-actions
Copy link
Copy Markdown
Contributor

github-actions bot commented Aug 8, 2023

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.

@georgewrmarshall georgewrmarshall added the team-design-system All issues relating to design system in Extension label Aug 10, 2023
@Jainex17
Copy link
Copy Markdown
Contributor Author

@NidhiKJha Could you please review this pr?

NidhiKJha
NidhiKJha previously approved these changes Aug 17, 2023
Copy link
Copy Markdown
Member

@NidhiKJha NidhiKJha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for your contribution

@Jainex17
Copy link
Copy Markdown
Contributor Author

@georgewrmarshall Could you please review this pr?

Copy link
Copy Markdown
Contributor

@georgewrmarshall georgewrmarshall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks for your contribution @Jainex17

@georgewrmarshall georgewrmarshall dismissed stale reviews from NidhiKJha and themself via 2145bf9 August 18, 2023 23:56
@georgewrmarshall
Copy link
Copy Markdown
Contributor

Still a few more selector updates that need to be made to get this to pass. Let me know if you need some assistance @Jainex17

@Jainex17
Copy link
Copy Markdown
Contributor Author

Still a few more selector updates that need to be made to get this to pass. Let me know if you need some assistance @Jainex17

Thanks, I'm still making an effort, but having some steps from you to resolve this build issue would be incredibly helpful.

Copy link
Copy Markdown
Contributor

@georgewrmarshall georgewrmarshall left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Hey @Jainex17, to get this PR in I reverted the changes to the token allowance files and we can open another PR to swap out the deprecated components. Thanks for your contribution

NidhiKJha
NidhiKJha previously approved these changes Oct 16, 2023
Copy link
Copy Markdown
Member

@NidhiKJha NidhiKJha left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

…s which should help us see all alerts in one view
@georgewrmarshall georgewrmarshall dismissed stale reviews from NidhiKJha and themself via 13cb809 October 16, 2023 12:00
Comment on lines +225 to +235
metamask: {
...testData.metamask,
// TODO: Mock state correctly to show insufficient funds error
//
accounts: {
...testData.metamask.accounts,
'0x9d0ba4ddac06032527b140912ec808ab9451b788': {
balance: '0x0',
},
},
},
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @matthewwalsh0, do you have any ideas on how one could show the insufficient funds for gas error? I seem to be struggling with that

Screenshot 2023-10-16 at 3 07 43 PM

Copy link
Copy Markdown
Member

@kumavis kumavis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thank you for this!
the After video shows a bug suggesting some type error: "Spending cap request for your [Object object]"

@digiwand
Copy link
Copy Markdown
Contributor

Awesome to see this working @Jainex17!

@kumavis
I think this is a non-blocker as it's an outstanding issue. Our storybook build does not appear to support i18n placeholder input and as a result we have other storybook pages that render "[Object object]"

I created a follow-up ticket for us to address here: #21405

@georgewrmarshall
Copy link
Copy Markdown
Contributor

georgewrmarshall commented Oct 18, 2023

I created a follow-up ticket for us to address here: #21405

Thanks @digiwand 🙏

kumavis
kumavis previously approved these changes Oct 19, 2023
@codecov
Copy link
Copy Markdown

codecov bot commented Oct 25, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (a3085c1) 68.82% compared to head (2c68512) 68.81%.

Additional details and impacted files
@@             Coverage Diff             @@
##           develop   #20422      +/-   ##
===========================================
- Coverage    68.82%   68.81%   -0.00%     
===========================================
  Files         1034     1034              
  Lines        41166    41166              
  Branches     10994    10994              
===========================================
- Hits         28329    28327       -2     
- Misses       12837    12839       +2     

see 2 files with indirect coverage changes

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

@Jainex17
Copy link
Copy Markdown
Contributor Author

@georgewrmarshall Sorry for being inactive is there anything i can do in this PR Thanks

@georgewrmarshall georgewrmarshall merged commit 874f63c into MetaMask:develop Jan 23, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Jan 23, 2024
@Jainex17 Jainex17 deleted the fix/17343/broken-story-TokenAllowance branch January 28, 2024 05:56
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

team-design-system All issues relating to design system in Extension

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Broken Story : TokenAllowance

6 participants