-
Notifications
You must be signed in to change notification settings - Fork 5k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix: Permit value decimal & refactor: Create reusable ConfirmInfoRowTextTokenUnits component #26105
Conversation
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Summary
The refactor and redesign of the confirmation token amount feature introduces new components and updates existing ones to improve functionality and maintainability.
- New Component: Added
ui/components/app/confirm/info/row/text-token.tsx
to format and display token amounts with precision. - Storybook Addition: Introduced
ui/components/app/confirm/info/row/text-token.stories.tsx
for visual testing of theConfirmInfoRowTextToken
component. - Unit Tests: Added
ui/components/app/confirm/info/row/text-token.test.tsx
to ensure correct rendering and formatting of token values. - Refactor: Updated
ui/pages/confirmations/components/confirm/info/typed-sign/permit-simulation/permit-simulation.tsx
to usecalcTokenAmount
for consistent token calculations. - Code Simplification: Modified
ui/pages/confirmations/components/confirm/row/dataTree.tsx
to replace custom formatting with the newConfirmInfoRowTextToken
component.
6 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Summary
(updates since last review)
The refactor and redesign of the confirmation token amount feature primarily involve renaming components and updating their usage to improve clarity and maintainability.
- Component Renaming:
ui/components/app/confirm/info/row/text-token.tsx
renamed totext-token-units.tsx
to better reflect its functionality. - Export Update: Modified
ui/components/app/confirm/info/row/index.ts
to exporttext-token-units
instead oftext-token
. - Story and Test Updates: Renamed
text-token.stories.tsx
andtext-token.test.tsx
totext-token-units.stories.tsx
andtext-token-units.test.tsx
respectively, aligning with the new component name. - DataTree Component Update: Adjusted
ui/pages/confirmations/components/confirm/row/dataTree.tsx
to useConfirmInfoRowTextTokenUnits
for better naming consistency.
5 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
Quality Gate passedIssues Measures |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Summary
(updates since last review)
The refactor introduces a reusable ConfirmInfoRowTextTokenUnits
component to streamline token amount display across the application.
- New Component: Added
ConfirmInfoRowTextTokenUnits
inui/components/app/confirm/info/row/text-token-units.tsx
. - Test Enhancements: Updated
shared/lib/transaction-controller-utils.test.js
to include precision handling tests. - Storybook Addition: Created
invalid-custom-network-alert.stories.tsx
for visual testing ofInvalidCustomNetworkAlert
. - E2E Test Reliability: Improved
test/e2e/accounts/create-snap-account.spec.ts
by adding wait conditions for popup handling. - UI Consistency: Modified
ui/pages/onboarding-flow/metametrics/index.scss
to maintain consistent button width across screen sizes.
12 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings
Builds ready [d1f422d]
Page Load Metrics (171 ± 196 ms)
Bundle size diffs
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #26105 +/- ##
===========================================
- Coverage 69.67% 69.67% -0.00%
===========================================
Files 1405 1406 +1
Lines 49733 49731 -2
Branches 13751 13751
===========================================
- Hits 34651 34649 -2
Misses 15082 15082 ☔ View full report in Codecov by Sentry. |
// FIXME - Precision may be lost for large values when using formatAmount | ||
/** @see {@link https://github.com/MetaMask/metamask-extension/issues/25755} */ | ||
const tokenValue = formatAmount('en-US', resultBn); | ||
const tokenValueMaxPrecision = formatAmountMaxPrecision('en-US', valueBN); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
valueBn should have been resultBn here. This caused the token decimal issue.
Description
No UI/UX changes
Create reusable ConfirmInfoRowTextTokenUnits
This also fixes the missing token decimals we had on the Permit value
notes:
Related issues
Fixes: https://github.com/MetaMask/MetaMask-planning/issues/2888
Manual testing steps
Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist