feat: Update Redesign Signature Permit to show ellipsis at max 15 digits by digiwand · Pull Request #26227 · MetaMask/metamask-extension

digiwand · 2024-07-30T15:40:38Z

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

Go to cowswap
Swap a token with gas-free approve for another token
Notice the permit signature screen

or

run yarn storybook
go to http://localhost:6006/?path=/story/pages-confirmations-confirm-signatures-signedtypeddatav3orv4--permit-story&args=msgParams.data:3.0001231231212312e+22
copy and paste the following into data

"{\"types\":{\"EIP712Domain\":[{\"name\":\"name\",\"type\":\"string\"},{\"name\":\"version\",\"type\":\"string\"},{\"name\":\"chainId\",\"type\":\"uint256\"},{\"name\":\"verifyingContract\",\"type\":\"address\"}],\"Permit\":[{\"name\":\"owner\",\"type\":\"address\"},{\"name\":\"spender\",\"type\":\"address\"},{\"name\":\"value\",\"type\":\"uint256\"},{\"name\":\"nonce\",\"type\":\"uint256\"},{\"name\":\"deadline\",\"type\":\"uint256\"}]},\"primaryType\":\"Permit\",\"domain\":{\"name\":\"MyToken\",\"version\":\"1\",\"verifyingContract\":\"0xCcCCccccCCCCcCCCCCCcCcCccCcCCCcCcccccccC\",\"chainId\":1},\"message\":{\"owner\":\"0x935e73edb9ff52e23bac7f7e043a1ecd06d05477\",\"spender\":\"0x5B38Da6a701c568545dCfcB03FcB875f56beddC4\",\"value\":30001231231212312138768,\"nonce\":0,\"deadline\":50000000000}}"

Screenshots/Recordings

Before

After

Pre-merge author checklist

I've followed MetaMask Contributor Docs and MetaMask Extension Coding Standards.
I've completed the PR template to the best of my ability
I’ve included tests if applicable
I’ve documented my code using JSDoc format if applicable
I’ve applied the right labels on the PR (see labeling guidelines). Not required for external contributors.

Pre-merge reviewer checklist

I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

github-actions · 2024-07-30T15:40:53Z

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.

sonarqubecloud · 2024-07-30T16:48:27Z

Quality Gate passed

Issues
1 New issue
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

metamaskbot · 2024-07-30T16:54:04Z

Builds ready [25b4f0a]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5
- common: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9
- content-script: 0
- offscreen: 0
- ui: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (72 ± 17 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	73	229	102	32	16
		domContentLoaded	9	170	30	34	16
		load	41	210	72	35	17
		domInteractive	9	169	30	34	16

Bundle size diffs

background: 0 Bytes (0.00%)
ui: 150 Bytes (0.00%)
common: 0 Bytes (0.00%)

metamaskbot · 2024-07-30T17:13:53Z

Builds ready [566166e]

builds: chrome, firefox
builds (beta): chrome
builds (flask): chrome, firefox
builds (MMI): chrome, firefox
builds (test): chrome, firefox
builds (test-flask): chrome, firefox
build viz: Build System
mv3: Background Module Init Stats
mv3: UI Init Stats
mv3: Module Load Stats
mv3: Bundle Size Stats
mv2: E2e Actions Stats
code coverage: Report
storybook: Storybook
typescript migration: Dashboard
all artifacts
bundle viz:
- background: 0, 1, 2, 3, 4, 5
- common: 0, 1, 2, 3, 4, 5, 6, 7, 8, 9
- content-script: 0
- offscreen: 0
- ui: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (294 ± 264 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	66	341	108	60	29
		domContentLoaded	8	108	26	21	10
		load	45	1707	294	550	264
		domInteractive	8	108	26	21	10

Bundle size diffs

background: 0 Bytes (0.00%)
ui: 150 Bytes (0.00%)
common: 0 Bytes (0.00%)

codecov · 2024-07-30T17:14:02Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 69.97%. Comparing base (aceea3b) to head (566166e).
Report is 11 commits behind head on develop.

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.
📢 Have feedback on the report? Share it here.

matthewwalsh0 · 2024-07-31T07:58:00Z

ui/pages/confirmations/components/simulation-details/formatAmount.ts

+  const [integerPart] = amountText.split('.');
+  const cleanIntegerPart = integerPart.replace(/,/gu, '');
+
+  if (cleanIntegerPart.length > maxLeftDigits) {


Could we re-use Goktug's shortenString function here?

From ui/helpers/utils/util.js.

I think it’s better to keep it as is since this requires more logic to count the digits and preserve the “,”

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

digiwand · 2024-08-16T01:11:24Z

Design set max 15 chars, not max 15 digits. Updated in new PR: #26458

digiwand added 3 commits July 30, 2024 18:24

feat: display ellipsis if > 15 digits permit amounts

a65d821

fix: rm no longer needed css ellipsis

e2a9718

refactor: remove unused isEllipsis from ConfirmInfoRowText

706d6d9

digiwand requested review from a team as code owners July 30, 2024 15:40

metamaskbot added the team-confirmations Push issues to confirmations team label Jul 30, 2024

digiwand changed the title ~~feat: Update ellipsis UI to display on Permit amounts at max 15 digits~~ feat: Update Redesign Signature Permit to show ellipsis at max 15 digits Jul 30, 2024

digiwand added 2 commits July 30, 2024 19:06

test:fix: snapshots related to permit ellipsis

25b4f0a

chore: rm stale comments

566166e

jpuri approved these changes Jul 31, 2024

View reviewed changes

matthewwalsh0 requested changes Jul 31, 2024

View reviewed changes

digiwand requested a review from matthewwalsh0 August 1, 2024 08:35

matthewwalsh0 approved these changes Aug 1, 2024

View reviewed changes

digiwand merged commit f001d60 into develop Aug 2, 2024

digiwand deleted the feat-permit-display-max-15-digits branch August 2, 2024 13:13

github-actions bot locked and limited conversation to collaborators Aug 2, 2024

metamaskbot added the release-12.4.0 Issue or pull request that will be included in release 12.4.0 label Aug 2, 2024

digiwand added release-12.1.0 Issue or pull request that will be included in release 12.1.0 rc-cherry-picked and removed release-12.4.0 Issue or pull request that will be included in release 12.4.0 labels Aug 20, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Update Redesign Signature Permit to show ellipsis at max 15 digits#26227

feat: Update Redesign Signature Permit to show ellipsis at max 15 digits#26227
digiwand merged 5 commits intodevelopfrom
feat-permit-display-max-15-digits

digiwand commented Jul 30, 2024 •

edited

Loading

Uh oh!

github-actions bot commented Jul 30, 2024

Uh oh!

sonarqubecloud bot commented Jul 30, 2024

Uh oh!

metamaskbot commented Jul 30, 2024

Uh oh!

metamaskbot commented Jul 30, 2024

Uh oh!

codecov bot commented Jul 30, 2024 •

edited

Loading

Uh oh!

matthewwalsh0 Jul 31, 2024

Uh oh!

digiwand Aug 1, 2024

Uh oh!

digiwand Aug 16, 2024

Uh oh!

digiwand commented Aug 16, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

digiwand commented Jul 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related issues

Manual testing steps

Screenshots/Recordings

Before

After

Pre-merge author checklist

Pre-merge reviewer checklist

Uh oh!

github-actions bot commented Jul 30, 2024

Uh oh!

sonarqubecloud bot commented Jul 30, 2024

Quality Gate passed

Uh oh!

metamaskbot commented Jul 30, 2024

Uh oh!

metamaskbot commented Jul 30, 2024

Uh oh!

codecov bot commented Jul 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

matthewwalsh0 Jul 31, 2024

Choose a reason for hiding this comment

Uh oh!

digiwand Aug 1, 2024

Choose a reason for hiding this comment

Uh oh!

digiwand Aug 16, 2024

Choose a reason for hiding this comment

Uh oh!

digiwand commented Aug 16, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

digiwand commented Jul 30, 2024 •

edited

Loading

codecov bot commented Jul 30, 2024 •

edited

Loading