- Notifications
You must be signed in to change notification settings - Fork5.4k
feat: Design upgrades for Confirmations#38399
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Open
dan437 wants to merge7 commits intomainChoose a base branch
base:main
Could not load branches
Branch not found:{{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline, and old review comments may become outdated.
Open
Uh oh!
There was an error while loading.Please reload this page.
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
Contributor
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. |
Collaborator
metamaskbot commentedNov 28, 2025 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
✨ Files requiring CODEOWNER review ✨✅@MetaMask/confirmations (8 files, +16 -16)
|
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Collaborator
metamaskbot commentedNov 28, 2025
Builds ready [f43f1ad]
UI Startup Metrics (1230 ± 110 ms)
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
f43f1ad tof6710deCompareUh oh!
There was an error while loading.Please reload this page.
…t if no inline alert textSigned-off-by: dan437 <80175477+dan437@users.noreply.github.com>
Signed-off-by: dan437 <80175477+dan437@users.noreply.github.com>
Signed-off-by: dan437 <80175477+dan437@users.noreply.github.com>
Signed-off-by: dan437 <80175477+dan437@users.noreply.github.com>
f6710de to3d1e7d3Comparematthewwalsh0 previously approved these changesNov 28, 2025
| backgroundColor={backgroundColor} | ||
| style={{ | ||
| cursor:onClick ?'pointer' :'default', | ||
| ...(!textOverride&&{backgroundColor:'transparent'}), |
Member
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, as we're adding more inline CSS, should we move this to a class?
...onfirmations/components/confirm/info/typed-sign-v1/__snapshots__/typed-sign-v1.test.tsx.snapShow resolvedHide resolved
Uh oh!
There was an error while loading.Please reload this page.
OGPoyraz previously approved these changesNov 28, 2025
Signed-off-by: dan437 <80175477+dan437@users.noreply.github.com>
| } | ||
| &__transparent-background { | ||
| background-color:transparent!important; |
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.
Bug: CSS important rule overrides backgroundColor prop
The!important rule onbackground-color: transparent will override thebackgroundColor prop passed to the component, even when explicitly set. WhentextOverride is falsy, theinline-alert__transparent-background class appliesbackground-color: transparent !important, which takes precedence over both thebackgroundColor prop and any inline styles. This prevents customizing the background color for icon-only alerts wherebackgroundColor is explicitly provided via props.
Collaborator
metamaskbot commentedNov 28, 2025
Builds ready [658febb]
UI Startup Metrics (1197 ± 98 ms)
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Signed-off-by: dan437 <80175477+dan437@users.noreply.github.com>
| 'inline-alert__success':severity===Severity.Success, | ||
| 'inline-alert__disabled':severity===Severity.Disabled, | ||
| 'inline-alert__pill':pill, | ||
| 'inline-alert__transparent-background':!textOverride, |
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.
Bug: Transparent background overrides explicit backgroundColor prop
Theinline-alert__transparent-background class is applied whentextOverride is falsy, including empty strings. This class uses!important to force transparency, which prevents thebackgroundColor prop from working even when explicitly provided. For example, malicious transactions setbackgroundColor toBackgroundColor.errorMuted while passing an empty string fortextOverride, but the transparent background will override this intended styling. The condition should check ifbackgroundColor is provided before applying the transparent class, or avoid using!important.
Collaborator
metamaskbot commentedNov 28, 2025
Builds ready [e4df7e9]
UI Startup Metrics (1226 ± 109 ms)
📊 Page Load Benchmark ResultsCurrent Commit: 📄 Localhost MetaMask Test DappSamples: 100 Summary
📈 Detailed Results
Bundle size diffs [🚨 Warning! Bundle size has increased!]
|
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading.Please reload this page.
Description
A few design upgrades for Confirmations:
Changelog
CHANGELOG entry: feat: Design upgrades for Confirmations
Related issues
Fixes:https://github.com/MetaMask/MetaMask-planning/issues/5279
Manual testing steps
Screenshots/Recordings
Before
After
Pre-merge author checklist
Pre-merge reviewer checklist
Note
Refines Confirmations UI by softening label color, using medium label weight, adding 16px icon spacing, and making icon-only inline alerts use a transparent background.
TextColor.textAlternativeand useTextVariant.bodyMdMediumfor headings (e.g.,From,To,Network fee, etc.).gap={4}between header info icons; remove explicit right margin fromAdvancedDetailsButton.textOverrideis provided; otherwise show icon-only.inline-alert__transparent-backgroundclass and apply it when no text is shown.inline-alert.BalanceChangeRowlabel defaults totextAlternativeandbodyMdMedium.Written byCursor Bugbot for commite4df7e9. This will update automatically on new commits. Configurehere.