-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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: SIWE sign message section #24997
Conversation
… permit_sign_info_section
… siwe_sign_simulation_section
… permit_sign_info_section
ui/pages/confirmations/components/confirm/info/personal-sign/personal-sign.tsx
Outdated
Show resolved
Hide resolved
ui/pages/confirmations/components/confirm/info/personal-sign/siwe-sign/siwe-sign.tsx
Outdated
Show resolved
Hide resolved
ui/pages/confirmations/components/confirm/info/personal-sign/siwe-sign/siwe-sign.tsx
Outdated
Show resolved
Hide resolved
ui/pages/confirmations/components/confirm/info/personal-sign/siwe-sign/siwe-sign.tsx
Outdated
Show resolved
Hide resolved
ui/pages/confirmations/components/confirm/info/personal-sign/siwe-sign/siwe-sign.tsx
Outdated
Show resolved
Hide resolved
ui/pages/confirmations/components/confirm/info/personal-sign/siwe-sign/siwe-sign.tsx
Outdated
Show resolved
Hide resolved
ui/pages/confirmations/components/confirm/info/personal-sign/siwe-sign/siwe-sign.stories.tsx
Show resolved
Hide resolved
ui/pages/confirmations/utils/date.ts
Outdated
if (!dateString) { | ||
return dateString; | ||
} | ||
const date = new Date(dateString); |
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 achieve this easier with Intl.DateTimeFormat.format
, plus it will be translatable?
There seems to be some existing usages in ui/helpers/utils/notification.util.ts
.
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 tried that, but I was facing issues in displayed same value of day in different locales.
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.
My worry is what we have currently is partially re-implementing that, plus we have no translation or locale support at all.
Can you share the issue concerning locales?
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.
Please check this broken test case: https://app.circleci.com/pipelines/github/MetaMask/metamask-extension/84899/workflows/000a7574-6f5e-4e06-bc7b-fd50448a2c3d/jobs/3066779
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've tested with that commit, and I believe the issue was just that we weren't explicit with the timezone.
Using the following in format date, the existing util and snapshot tests passed without any changes to them:
export const formatDate = (dateString: string) => {
if (!dateString) {
return dateString;
}
return DateTime.fromISO(dateString)
.setLocale('en')
.setZone('utc')
.toFormat('dd LLLL yyyy, HH:mm');
};
We can of course choose to use the user's timezone and or their locale, but as a first step, this means we don't rely on any custom format logic and have easy configuration of the output.
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.
Good suggestion, I updated the PR.
…ask-extension into siwe_sign_message_section
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #24997 +/- ##
===========================================
- Coverage 65.65% 65.62% -0.02%
===========================================
Files 1368 1375 +7
Lines 54269 54544 +275
Branches 14192 14291 +99
===========================================
+ Hits 35627 35794 +167
- Misses 18642 18750 +108 ☔ View full report in Codecov by Sentry. |
Builds ready [de2aca0]
Page Load Metrics (124 ± 144 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
…ask-extension into siwe_sign_message_section
New dependencies detected. Learn more about Socket for GitHub ↗︎
|
…ask-extension into siwe_sign_message_section
Builds ready [9e42c01]
Page Load Metrics (49 ± 3 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Description
Changes in message section for SIWE signature pages.
Related issues
Fixes: #24650
Manual testing steps
Screenshots/Recordings
Pre-merge author checklist
Pre-merge reviewer checklist