New SecurityProviderBannerAlert component #19825
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. |
Builds ready [97d7355]
Page Load Metrics (1602 ± 46 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Codecov Report
@@ Coverage Diff @@
## develop #19825 +/- ##
===========================================
+ Coverage 69.36% 69.37% +0.01%
===========================================
Files 986 987 +1
Lines 37279 37286 +7
Branches 10010 10012 +2
===========================================
+ Hits 25857 25867 +10
+ Misses 11422 11419 -3
|
digiwand
added
the
team-confirmations-secure-ux-PR
Builds ready [20c1b8e]
Page Load Metrics (1729 ± 64 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Builds ready [eb13162]
Page Load Metrics (1521 ± 46 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Builds ready [8af4e6f]
Page Load Metrics (1949 ± 128 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
ui/components/app/security-provider-banner-alert/security-provider-banner-alert.js
Outdated
Show resolved
Hide resolved
ui/components/app/security-provider-banner-alert/security-provider-banner-alert.stories.js
Outdated
Show resolved
Hide resolved
ui/components/app/security-provider-banner-alert/security-provider-banner-alert.stories.js
Outdated
Show resolved
Hide resolved
| <Text variant={TextVariant.bodySm} as="h6" marginTop={2}> | ||
| {description} | ||
| </Text> |
There was a problem hiding this comment.
Could you use the description prop inside the BannerAlert? I think it should be bodyMd? Our typography is responsive so if you see 14px the design might be using the small screen typography. It should be 16px on larger screens. Also I know a lot of the Text are rendering as h6 this is probably not the best semantic mark up but I think was a decision instead of getting new contributors to update e2e tests. Probably not the best in hindsight. I think it could just be a p tag which is what bodySm renders by default
There was a problem hiding this comment.
I see now. description is inherited from BannerBase. Got it
I was carrying over code and preserving styles from our OpenSea SecurityProviderMessage component. These are great updates 👍 Thank you!
applied in 647eda9 and a7fdeee
I also adjusted the Disclosure component text size here 8ac9248
One thing using the description param is that is a lack of padding between the title and description which does not match the figma design. Any suggestions? @georgewrmarshall
There was a problem hiding this comment.
before:
after using description param:
There was a problem hiding this comment.
Hey @georgewrmarshall! hope you had a nice weekend. In addition to the lack of padding/margin between the title and the description, I realized there is another issue we have using the description BannerAlert/BannerBase property instead of passing children elements.
The description currently expects a string property. The problem is that SecurityProviderBannerAlert will allow hyperlinks, or other HTML elements, in the description.
error:
BannerBase > checkPropTypes.js:20 Warning: Failed prop type: Invalid prop
descriptionof typeobjectsupplied toBannerAlert
Do you have suggestions to address the two issues above?
There was a problem hiding this comment.
In the meantime I have reverted the use of description param to fix the console and UI issues. I think it would be better to address this in a separate PR to unblock dependent work ahead
Revert commit 1c3d550
There was a problem hiding this comment.
I am thinking, maybe, we can do something similar to our plans with the Disclosure component and add support using Text as=“div”
Builds ready [1deac07]
Page Load Metrics (1717 ± 65 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
| /** | ||
| * @typedef {object} SecurityProviderConfig | ||
| * @property {string} tKeyName - translation key for security provider name | ||
| * @property {string} url - URL to securty provider website | ||
| */ | ||
|
|
||
| /** @type {Record<string, SecurityProviderConfig>} */ | ||
| export const SECURITY_PROVIDER_CONFIG = { | ||
| [SecurityProvider.Blockaid]: { | ||
| tKeyName: 'blockaid', | ||
| url: 'https://blockaid.io/', | ||
| }, | ||
| }; |
There was a problem hiding this comment.
Besides the jsdoc why not create actual type and benefit from the full extent of TS? You could have something like:
type SecurityProviderConfig = Record<
SecurityProvider,
{
tKeyName: string;
url: string;
}
>;There was a problem hiding this comment.
lgtm! I was following patterns from shared/constants/gas.ts and other files. updated here fac7473
does this look okay or are there any more suggestions?
cc: @cryptotavares
There was a problem hiding this comment.
[nit]
Do we want this const to be readonly (at least for TS)? If yes then we can:
const SECURITY_PROVIDER_CONFIG: Readonly<SecurityProviderConfig>or ditch the type and use const assertion (TS docs for readonly and const assertion)
export const SECURITY_PROVIDER_CONFIG = {
[SecurityProvider.Blockaid]: {
tKeyName: 'blockaid',
url: 'https://blockaid.io/',
},
} as const;Using the const assertion seems more inline with what is being done on other constants TS files (example).
(Sorry for asking to add the type and then replacing it for const assertion.. I am mostly used to use the Readonly mapped type, but that does not seem to be the standard on the Extension code 😅 )
There was a problem hiding this comment.
hey @cryptotavares, thanks for the suggestion and references! I applied the const assertion and I am thinking the type you suggested earlier is also nice to help define the types for each of the config objects. (Before, I was using jsdoc @typedef instead of the type you first proposed.)
There was a problem hiding this comment.
If you use the const assertion TS should automatically pick the correct type.
If you do const assertion and type the const at the same time, then TS will interpret the explicit type as the actual type (meaning that if you do not add the readonly utility type, then it will allow overriding the current property values). Example:
// Just using const assertion
const test = {
yolo: 1
} as const;
test.yolo = 2; // TS will complain about this because when we asserted to const, test is readonlytype Test = {
yolo: number
}
// explicit typing with const assertion
const test: Test = {
yolo: 1
} as const;
test.yolo = 2; // TS will accept this, as it considers that test.yolo is a number and is not readonlyAnyway, I was suggesting that you can either use const assertion or you can explicitly type it (and use the Readonly utility type). But as const assertion seems to be the pattern used in the rest of the codebase you can do just that.
Btw, other examples on how we can force it to be readonly with explicit typing:
// Using Readonly utility type ensures that all properties are readonly and cannot be reassigned.
const test: Readonly<Test> = {
yolo: 1
};const test: Test = {
// using readonly modifier ensures that that specific property is readonly and cannot be reassigned
readonly yolo: 1
};There was a problem hiding this comment.
ooh I see now. I think I got it #19825 cc: @cryptotavares
Builds ready [3aea156]
Page Load Metrics (1474 ± 32 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Builds ready [dbfc92e]
Page Load Metrics (1580 ± 23 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
|
|
||
| /** | ||
| * Optional | ||
| */ |
There was a problem hiding this comment.
I thought it would be nice to separate the optional / not isRequired parameters
DISCLOSURE_TYPE 0-> DisclosureVariant
Co-authored-by: George Marshall <[email protected]>
Co-authored-by: George Marshall <[email protected]>
This reverts commit 647eda9.
digiwand
force-pushed
the
new-security-provider-banner-alert-component
branch
from
78c0c17 to
0e151d5
Compare
|
@jpuri @cryptotavares @georgewrmarshall sorry had to force push to resolve a rebase conflict. Should be the same. I double-checked that the code works as expected in storybook. may I get a re-approval please? |
Builds ready [0e151d5]
Page Load Metrics (1655 ± 40 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
metamaskbot
added
the
release-10.36.0
Gudahtt
added
release-11.1.0
Explanation
Fixes: https://github.com/MetaMask/MetaMask-planning/issues/558
Blocked By: #19768
Blocks: #19257
We will be replacing the OpenSea security provider warning message. The current SecurityProviderBannerMessage component that displays this has specific logic for OpenSea. This PR creates a new UI component that allows for scalability and general use cases. UI differences from the OpenSea provider warning message include the option to include details, an updated footer copy, and a SecurityTick icon.
Screenshots/Screencaps
New Component
Manual Testing Steps
yarn storybookPre-merge author checklist
Pre-merge reviewer checklist
If further QA is required (e.g. new feature, complex testing steps, large refactor), add the
Extension QA Boardlabel.In this case, a QA Engineer approval will be be required.