-
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: Add first time interaction warning #28435
feat: Add first time interaction warning #28435
Conversation
package.json
Outdated
@@ -256,7 +256,8 @@ | |||
"@ledgerhq/evm-tools/axios": "^0.28.0", | |||
"@ledgerhq/hw-app-eth/axios": "^0.28.0", | |||
"@ledgerhq/hw-app-eth@npm:^6.39.0": "patch:@ledgerhq/hw-app-eth@npm%3A6.39.0#~/.yarn/patches/@ledgerhq-hw-app-eth-npm-6.39.0-866309bbbe.patch", | |||
"@ledgerhq/evm-tools@npm:^1.2.3": "patch:@ledgerhq/evm-tools@npm%3A1.2.3#~/.yarn/patches/@ledgerhq-evm-tools-npm-1.2.3-414f44baa9.patch" | |||
"@ledgerhq/evm-tools@npm:^1.2.3": "patch:@ledgerhq/evm-tools@npm%3A1.2.3#~/.yarn/patches/@ledgerhq-evm-tools-npm-1.2.3-414f44baa9.patch", | |||
"@metamask/transaction-controller@^38.3.0": "npm:@metamask-previews/[email protected]" |
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.
Reminder to not forget to remove preview version
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.
Removed
…ddress-for-the-first-time
Builds ready [b1764e7]
Page Load Metrics (2157 ± 130 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
}} | ||
> | ||
{/* Intentional fragment */} | ||
<></> |
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.
Minor, can we make the children optional within ConfirmInfoAlertRow
or does this force some kind of spacing?
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.
Done
const t = useI18nContext(); | ||
const { currentConfirmation } = useConfirmContext(); | ||
|
||
const { isFirstTimeInteraction } = currentConfirmation as TransactionMeta; |
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.
Minor, useConfirmContext
accepts a template so we can avoid the cast.
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.
Done
app/_locales/en/messages.json
Outdated
@@ -2091,6 +2091,12 @@ | |||
"message": "File import not working? Click here!", | |||
"description": "Helps user import their account from a JSON file" | |||
}, | |||
"firstTimeInteractionReason": { |
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.
Minor, I think we try and standardise these with alertReasonX
and alertMessageX
.
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.
Thanks for the heads up
).toEqual([]); | ||
}); | ||
|
||
it('returns no alerts if firstTimeInteraction is false', () => { |
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.
Minor, is it worth an undefined
test also?
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.
Done
…ddress-for-the-first-time
Builds ready [2285d3c]
Page Load Metrics (1829 ± 97 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Builds ready [8afa23c]
Page Load Metrics (1495 ± 41 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
…ddress-for-the-first-time
style={{ | ||
flexDirection: FlexDirection.Column, | ||
alignItems: AlignItems.flexStart, |
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 this to be sure, this is not needed.
Builds ready [42ce181]
Page Load Metrics (1706 ± 82 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Builds ready [c60c010]
Page Load Metrics (1724 ± 68 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Builds ready [7801103]
Page Load Metrics (1699 ± 65 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Description
This PR implements first time interaction feature where it shows an alert if you interact with the address for the first time.
Information of the first time interaction is fetched in the transaction controller when the transaction is added to the state.
Core PR: MetaMask/core#4895
Related issues
Fixes: https://github.com/MetaMask/MetaMask-planning/issues/3040
Manual testing steps
Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist