Skip to content
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(snaps): Enable destructive footer buttons in Snap UI #29966

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

david0xd
Copy link
Contributor

@david0xd david0xd commented Jan 29, 2025

Description

Add/enable destructive (danger) button in Snaps UI custom footer.

Open in GitHub Codespaces

Related issues

Fixes: MetaMask/snaps#3015

Manual testing steps

  1. Install a Snap with footer buttons that have destructive variant.
  2. Confirm that the buttons look as expected.
  3. Make a button that has both states, destructive and disabled and check if it's rendered correctly.
  4. Try different variants with non-destructive buttons and confirm that there are no regressions.

Example:

<Footer>
    <Button name="cancel" variant="destructive">
      Cancel
    </Button>
    <Button name="confirm" variant="destructive" disabled={true}>
      Confirm
    </Button>
</Footer>

Screenshots/Recordings

Before

Screenshot 2025-01-29 at 15 45 55
Screenshot 2025-01-29 at 15 46 47
Screenshot 2025-01-29 at 15 48 12
Screenshot 2025-01-29 at 15 53 07

After

Screenshot 2025-01-29 at 15 29 22
Screenshot 2025-01-29 at 15 24 24
Screenshot 2025-01-29 at 15 32 12
Screenshot 2025-01-29 at 16 01 07

Pre-merge author checklist

Pre-merge reviewer checklist

  • I've manually tested the PR (e.g. pull and build branch, run the app, test code being changed).
  • I confirm that this PR addresses all acceptance criteria described in the ticket it closes and includes the necessary testing evidence such as recordings and or screenshots.

@david0xd david0xd added the team-snaps-platform Snaps Platform team label Jan 29, 2025
@david0xd david0xd self-assigned this Jan 29, 2025
Copy link
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.

@david0xd david0xd force-pushed the dd/snap-destructive-footer-button branch from 62ea315 to f47b917 Compare January 29, 2025 14:54
@david0xd david0xd marked this pull request as ready for review January 29, 2025 15:04
@david0xd david0xd requested a review from a team as a code owner January 29, 2025 15:04
@metamaskbot
Copy link
Collaborator

Builds ready [f47b917]
Page Load Metrics (1653 ± 51 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint15151903165210751
domContentLoaded15011842162710048
load15161905165310751
domInteractive249139209
backgroundConnect106623178
firstReactRender15104423115
getState472202211
initialActions01000
loadScripts1080132511927737
setupStore769232110
uiStartup172126061964230110
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 80 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@@ -88,6 +90,7 @@ export const SnapUIFooterButton: FunctionComponent<
flexDirection: FlexDirection.Row,
}}
data-theme={null}
danger={variant === 'destructive'}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems that this component uses a different color for the destructive state.

I think we want to mirror the confirmations screen, see below for comparison.

image
image

@@ -77,9 +77,10 @@ export const footer: UIComponentFactory<FooterElement> = ({
props: {
...buttonMapped.props,
variant:
providedChildren.length === 2 && index === 0
buttonMapped.props?.variant ??
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@GuillaumeRx Should take a look at this, I'm assuming we weren't intending to let users override this in all cases

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
team-snaps-platform Snaps Platform team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a destructive state to our footer action buttons
3 participants