-
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
feat: Update Redesign Signature Permit to show ellipsis at max 15 digits #26227
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. |
Quality Gate passedIssues Measures |
Builds ready [25b4f0a]
Page Load Metrics (72 ± 17 ms)
Bundle size diffs
|
Builds ready [566166e]
Page Load Metrics (294 ± 264 ms)
Bundle size diffs
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #26227 +/- ##
===========================================
+ Coverage 69.96% 69.97% +0.01%
===========================================
Files 1411 1411
Lines 49946 49960 +14
Branches 13805 13807 +2
===========================================
+ Hits 34942 34956 +14
Misses 15004 15004 ☔ View full report in Codecov by Sentry. |
const [integerPart] = amountText.split('.'); | ||
const cleanIntegerPart = integerPart.replace(/,/gu, ''); | ||
|
||
if (cleanIntegerPart.length > maxLeftDigits) { |
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.
Could we re-use Goktug's shortenString
function here?
From ui/helpers/utils/util.js
.
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.
I think it’s better to keep it as is since this requires more logic to count the digits and preserve the “,”
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.
Sorry, I lost the design and issue ticket for this and ended up created a new ticket. Now that I've seen the relevant issue, we needed to do max 15 characters and not digits
https://github.com/MetaMask/MetaMask-planning/issues/2845
Updated to use shortenString
here: #26458
Design set max 15 chars, not max 15 digits. Updated in new PR: #26458 |
Description
Related issues
Fixes: https://github.com/MetaMask/MetaMask-planning/issues/2845 (Permit design review)
Fixes: #26226 (Ellipsis fix)
Fixes: https://github.com/MetaMask/MetaMask-planning/issues/2730 (older, original issue)
Follow-up Cherry-picked PR:
#26500
Manual testing steps
or
yarn storybook
Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist