-
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
fix: Add origin pill to wallet_addEthereumChain confirmation #29317
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 [42229a6]
Page Load Metrics (1686 ± 47 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
42229a6
to
d61e63f
Compare
Builds ready [d61e63f]
Page Load Metrics (1763 ± 99 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
@@ -86,6 +87,7 @@ export const safeComponentList = { | |||
Typography, | |||
SmartTransactionStatusPage, | |||
UrlIcon, | |||
OriginPill, |
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.
You can see the moment we lost our beautiful alphabetical order 😄
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.
Reordered!
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.
Does this not already exist elsewhere that we can reuse, or is it part of another component?
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.
This does not exist in this configuration. UrlIcon with a Text component exists elsewhere but it's not wrapped in its own component and has been deprecated.
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.
TypeScript for all new components?
store, | ||
); | ||
|
||
expect(container).toMatchSnapshot(); |
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.
Did we say we were going to avoid adding new snapshot tests?
Could we verify just the text itself for example?
d61e63f
to
22f0e33
Compare
); | ||
} | ||
|
||
OriginPill.propTypes = { |
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.
This is only needed for JS components so can be removed.
Builds ready [22f0e33]
Page Load Metrics (1665 ± 94 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
6bf8145
22f0e33
to
6bf8145
Compare
Builds ready [fa1ac36]
Page Load Metrics (1648 ± 86 ms)
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Description
Adds Origin Pill component and references it on the confirmation template.
Related issues
Fixes: #26656
Manual testing steps
Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist