Replace ActionableMessage components with BannerAlerts in SIWE Sign-in with Ethereum page by digiwand · Pull Request #18207 · MetaMask/metamask-extension

digiwand · 2023-03-16T23:53:47Z

Explanation

Fixes #18206

.mm-icon Icon component uses a element which was not supported by ActionableMessage
Replace SIWE ActionableMessage usage with BannerAlert
- text in alerts are slightly larger due to the change
Add @deprecate jsdoc comment to ActionableMessage

Screenshots/Screencaps

Before (v10.26.1)

(actually, the icon locations were temp fixed in this hotfix #18200 just before this PR.)

After

Manual Testing Steps

Login to the extension
Navigate to the test dapp
Click Sign In With Ethereum (Bad Account)
Scroll to the bottom and observe the warning
Check other ActionableMessage components

Pre-merge author checklist

I've clearly explained:
- What problem this PR is solving
- How this problem was solved
- How reviewers can test my changes
Sufficient automated test coverage has been added

Pre-merge reviewer checklist

Manual testing (e.g. pull and build branch, run in browser, test code being changed)
PR is linked to the appropriate GitHub issue
IF this PR fixes a bug in the release milestone, add this PR to the release milestone

If further QA is required (e.g. new feature, complex testing steps, large refactor), add the Extension QA Board label.

In this case, a QA Engineer approval will be be required.

- unblocks mismatched domain support - we may re-add error handling here #18184 - reverts logic from #16616

…ning-ui

- apply to SIWE actionable-message - .mm-icon is a

github-actions · 2023-03-16T23:54:00Z

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.

This reverts commit c80a4a2.

georgewrmarshall

We could deprecate ActionableMessage for BannerAlert would it be possible to add a deprecation message above the component as well as this fix?

in ui/components/ui/actionable-message/actionable-message.js

/**
 * @deprecated `<ActionableMessage />` has been deprecated in favour of the `<BannerAlert />` component in ./ui/components/component-library/banner-alert/banner-alert.js
 *
 * See storybook documentation for Text here https://metamask.github.io/metamask-storybook/?path=/docs/components-componentlibrary-banneralert--default-story#banneralert
 *
 * Help to replace `ActionableMessage` with `BannerAlert` by submitting a PR
 */

metamaskbot · 2023-03-17T00:33:48Z

Builds ready [e65a1a1]

builds: chrome, firefox
builds (beta): chrome, firefox
builds (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
- common: 0, 1, 2, 3, 4, 5
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1670 ± 65 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	96	314	130	45	21
		domContentLoaded	1432	1887	1625	109	52
		load	1500	1934	1670	136	65
		domInteractive	1432	1887	1625	109	52

Bundle size diffs [🚀 Bundle size reduced!]

background: -176 bytes
ui: 19 bytes
common: 0 bytes

…ning-ui

…update-siwe-actionable-msg-and-support-mm-icon-in-icon-comp

…-icon-in-icon-comp

digiwand · 2023-03-17T18:23:57Z

Thanks @georgewrmarshall! BannerAlerts are looking much better in the UI and code 👍 I added the deprecation warning to the ActionableMessage component

metamaskbot · 2023-03-17T18:29:08Z

Builds ready [893ee6f]

builds: chrome, firefox
builds (beta): chrome, firefox
builds (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
- common: 0, 1, 2, 3, 4, 5
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1596 ± 35 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	100	173	121	18	8
		domContentLoaded	1472	1685	1583	65	31
		load	1472	1766	1596	73	35
		domInteractive	1472	1685	1583	65	31

Bundle size diffs

background: 0 bytes
ui: -518 bytes
common: 0 bytes

codecov · 2023-03-17T18:29:27Z

Codecov Report

Merging #18207 (d7ab254) into develop (1d5e8a7) will increase coverage by 0.21%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           develop   #18207      +/-   ##
===========================================
+ Coverage    64.01%   64.22%   +0.21%     
===========================================
  Files          918      918              
  Lines        35310    35310              
  Branches      8993     8993              
===========================================
+ Hits         22602    22675      +73     
+ Misses       12708    12635      -73

Impacted Files	Coverage Δ
...onents/ui/actionable-message/actionable-message.js	`82.22% <ø> (ø)`
...p/signature-request-siwe/signature-request-siwe.js	`67.39% <100.00%> (+65.22%)`	⬆️

... and 6 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

metamaskbot · 2023-03-18T01:08:12Z

Builds ready [c627db5]

builds: chrome, firefox
builds (beta): chrome, firefox
builds (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
- common: 0, 1, 2, 3, 4, 5
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 10, 11, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1791 ± 78 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	106	249	135	29	14
		domContentLoaded	1503	2114	1782	159	76
		load	1503	2114	1791	162	78
		domInteractive	1503	2114	1782	159	76

Bundle size diffs

background: 0 bytes
ui: -518 bytes
common: 0 bytes

georgewrmarshall

Looking great! One small suggestion

ui/components/app/signature-request-siwe/signature-request-siwe.js

georgewrmarshall · 2023-03-18T17:23:37Z

ui/components/app/signature-request-siwe/signature-request-siwe.js

+          <Text variant={TextVariant.bodyMdBold}>
+            {t('SIWEDomainInvalidTitle')}
+          </Text>{' '}
+          <Text variant={TextVariant.bodyMd}>{t('SIWEDomainInvalidText')}</Text>


Note to DS team to allow for ` tags cc @garrettbear so we could do something like

<Text> {t('SIWEDomainInvalidTitle')} {t('SIWEDomainInvalidText')} </Text>

ui/components/app/signature-request-siwe/signature-request-siwe.js

…e.js Co-authored-by: George Marshall <george.marshall@consensys.net>

…-icon-in-icon-comp

georgewrmarshall

LGTM! Just need to fix a linting issue that was probably introduced by my suggestions apologies!

digiwand · 2023-03-20T19:36:39Z

Thanks @georgewrmarshall! fixed de6a131

…-icon-in-icon-comp

metamaskbot · 2023-03-20T20:04:55Z

Builds ready [d7ab254]

builds: chrome, firefox
builds (beta): chrome, firefox
builds (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
- common: 0, 1, 2, 3, 4, 5
- content-script: 0
- contentscript: contentscript
- disable-console: disable-console
- inpage: inpage
- policy-load: policy-load
- sentry-install: sentry-install
- ui: 0, 1, 10, 2, 3, 4, 5, 6, 7, 8, 9

Page Load Metrics (1602 ± 57 ms)

Platform	Page	Metric	Min (ms)	Max (ms)	Average (ms)	StandardDeviation (ms)	MarginOfError (ms)
Chrome	Home	firstPaint	98	136	115	12	6
		domContentLoaded	1424	1854	1578	108	52
		load	1424	1854	1602	118	57
		domInteractive	1424	1854	1578	108	52

Bundle size diffs

background: 0 bytes
ui: -502 bytes
common: 0 bytes

georgewrmarshall

LGTM! 🚀

digiwand and others added 7 commits March 16, 2023 10:14

siwe: re-enable warning UI for mismatched domains

d924916

- unblocks mismatched domain support - we may re-add error handling here #18184 - reverts logic from #16616

siwe: fix mismatch domain warning msg UI

9fb40be

lint: rm whitespace EOL

da120ff

siwe: rm unit test

c80a4a2

lint: fix whitespace

bdbc997

Merge branch 'develop' into fix-siwe-unblock-mismatch-domain-with-war…

185213d

…ning-ui

Icon: support .mm-icon

0f0b31c

- apply to SIWE actionable-message - .mm-icon is a

digiwand requested a review from a team as a code owner March 16, 2023 23:53

digiwand requested a review from ryanml March 16, 2023 23:53

digiwand changed the base branch from develop to fix-siwe-unblock-mismatch-domain-with-warning-ui March 16, 2023 23:55

digiwand added 2 commits March 16, 2023 17:05

lint: fix newline

e65a1a1

Revert "siwe: rm unit test"

412a91a

This reverts commit c80a4a2.

digiwand mentioned this pull request Mar 17, 2023

Sign in with Ethereum: re-enable warning UI for mismatched domains / disable domain binding #18200

Merged

8 tasks

georgewrmarshall reviewed Mar 17, 2023

View reviewed changes

digiwand added 6 commits March 16, 2023 17:43

ActionableMessage: add deprecation

3aed558

siwe: replace actionable-msg w/ banner-alert

2f4e00c

Merge branch 'develop' into fix-siwe-unblock-mismatch-domain-with-war…

e11c2fc

…ning-ui

ActionableMessage: add param for lint

daa5838

Merge branch 'fix-siwe-unblock-mismatch-domain-with-warning-ui' into …

4e34c4d

…update-siwe-actionable-msg-and-support-mm-icon-in-icon-comp

revert .mm_icon ActionableMessage support

c89680b

Base automatically changed from fix-siwe-unblock-mismatch-domain-with-warning-ui to develop March 17, 2023 17:36

Merge branch 'develop' into update-siwe-actionable-msg-and-support-mm…

893ee6f

…-icon-in-icon-comp

digiwand added 3 commits March 17, 2023 17:16

siwe: create tests

45b273a

siwe: fix typo in tests

f9c8209

siwe: fix - do not allow nested elements

c627db5

georgewrmarshall reviewed Mar 18, 2023

View reviewed changes

digiwand and others added 3 commits March 18, 2023 14:16

Update ui/components/app/signature-request-siwe/signature-request-siw…

ee0ce10

…e.js Co-authored-by: George Marshall <george.marshall@consensys.net>

Update ui/components/app/signature-request-siwe/signature-request-siw…

9b98162

…e.js Co-authored-by: George Marshall <george.marshall@consensys.net>

Merge branch 'develop' into update-siwe-actionable-msg-and-support-mm…

0832e3e

…-icon-in-icon-comp

georgewrmarshall reviewed Mar 20, 2023

View reviewed changes

eslint fix

de6a131

digiwand dismissed a stale review via de6a131 March 20, 2023 19:30

digiwand requested a review from georgewrmarshall March 20, 2023 19:35

Merge branch 'develop' into update-siwe-actionable-msg-and-support-mm…

d7ab254

…-icon-in-icon-comp

digiwand changed the title ~~ActionableMessage component styles do not support Icon component (.mm-icon)~~ Replace ActionableMessage components with BannerAlerts in SIWE Sign-in with Ethereum page Mar 20, 2023

georgewrmarshall approved these changes Mar 20, 2023

View reviewed changes

jpuri approved these changes Mar 21, 2023

View reviewed changes

digiwand merged commit 5e3770e into develop Mar 21, 2023

digiwand deleted the update-siwe-actionable-msg-and-support-mm-icon-in-icon-comp branch March 21, 2023 16:49

github-actions bot locked and limited conversation to collaborators Mar 21, 2023

digiwand added team-confirmations-secure-ux-PR PRs from the confirmations team area-signatures labels Mar 24, 2023

Conversation

digiwand commented Mar 16, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Explanation

Screenshots/Screencaps

Before (v10.26.1)

After

Manual Testing Steps

Pre-merge author checklist

Pre-merge reviewer checklist

Uh oh!

github-actions bot commented Mar 16, 2023

Uh oh!

georgewrmarshall left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

metamaskbot commented Mar 17, 2023

Uh oh!

digiwand commented Mar 17, 2023

Uh oh!

metamaskbot commented Mar 17, 2023

Uh oh!

codecov bot commented Mar 17, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

metamaskbot commented Mar 18, 2023

Uh oh!

georgewrmarshall left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

georgewrmarshall Mar 18, 2023

Choose a reason for hiding this comment

Uh oh!

Uh oh!

georgewrmarshall left a comment

Choose a reason for hiding this comment

Uh oh!

digiwand commented Mar 20, 2023

Uh oh!

metamaskbot commented Mar 20, 2023

Uh oh!

georgewrmarshall left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

digiwand commented Mar 16, 2023 •

edited

Loading

georgewrmarshall left a comment •

edited

Loading

codecov bot commented Mar 17, 2023 •

edited

Loading