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): Add background color prop to Snaps UI Container component #29095

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

david0xd
Copy link
Contributor

@david0xd david0xd commented Dec 11, 2024

Description

This PR adds backgroundColor property to Container component.
Background colors that can be used are predefined as: default and alternative.
This PR deliberately disables the backgroundColor feature for containers used within Transaction Insights pages (Snaps).

Open in GitHub Codespaces

Related issues

Fixes: MetaMask/snaps#2906
Related Snaps PR: MetaMask/snaps#2950

Manual testing steps

  1. Try installing and using a Snap with confirmation window or home page, with Container in its content with backgroundColor defined (e.g. <Container backgroundColor="default">).
  2. Try the same thing on transaction insights Snaps and confirm that the colors are not changing there.
  3. Confirm that the colors are as expected on all pages.

Some source code used for testing:

  • Source code used for confirmation content:
      return snap.request({
        method: 'snap_dialog',
        params: {
          content: (
            <Container backgroundColor="default">
              <Box>
                <Text>Testing container background.</Text>
                <Button variant="primary">Primary button</Button>
                <Button variant="destructive">Destructive button</Button>
                <Button disabled={true}>Disabled button</Button>
              </Box>
              <Footer>
                <Button>Accept</Button>
                <Button name="footer_button">Cancel</Button>
              </Footer>
            </Container>
          ),
        },
      });

Source code used for Transaction Insight content:

    return {
      content: (
        <Container backgroundColor="alternative">
          <Box>
            <Text>Testing container background on transaction insight.</Text>
            <Text>Normal transaction.</Text>
          </Box>
        </Container>
      ),
      severity: SeverityLevel.Critical,
    };

Screenshots/Recordings

Before

Note: Changing container colors was not possible before.
Screenshot 2024-12-11 at 14 06 34
Screenshot 2024-12-11 at 14 06 59

After

Here are some examples when colors are changed (specified as a prop on container).
Screenshot 2024-12-11 at 13 41 22
Screenshot 2024-12-11 at 13 37 51
Screenshot 2024-12-11 at 13 36 57

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 Dec 11, 2024
@david0xd david0xd self-assigned this Dec 11, 2024
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/add-snap-ui-container-bg branch from f83bcff to 0f5f5d3 Compare January 10, 2025 10:37
@metamaskbot
Copy link
Collaborator

Builds ready [0f5f5d3]
Page Load Metrics (1675 ± 43 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint1500184116859948
domContentLoaded1487179316508943
load1549183716758943
domInteractive24172564321
backgroundConnect997282412
firstReactRender16101483416
getState566192110
initialActions01000
loadScripts1115137012367335
setupStore67315189
uiStartup174724722035231111
Bundle size diffs
  • background: 0 Bytes (0.00%)
  • ui: 376 Bytes (0.00%)
  • common: 0 Bytes (0.00%)

@metamaskbot
Copy link
Collaborator

Builds ready [fdf9081]
Page Load Metrics (1629 ± 43 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint1525184916369546
domContentLoaded1481178916048842
load1524180716298943
domInteractive24218524622
backgroundConnect985262110
firstReactRender1691562813
getState595152110
initialActions01000
loadScripts1068137811808240
setupStore791212412
uiStartup17292365196919795
Bundle size diffs [🚀 Bundle size reduced!]
  • background: 0 Bytes (0.00%)
  • ui: 504 Bytes (0.01%)
  • common: -4.81 KiB (-0.06%)

@metamaskbot
Copy link
Collaborator

Builds ready [406ca2d]
Page Load Metrics (1811 ± 85 ms)
PlatformPageMetricMin (ms)Max (ms)Average (ms)StandardDeviation (ms)MarginOfError (ms)
ChromeHomefirstPaint15902271181518187
domContentLoaded15592249177817383
load15982274181117785
domInteractive27110442211
backgroundConnect880362211
firstReactRender16103623517
getState691333014
initialActions01000
loadScripts11371732132615273
setupStore78015209
uiStartup183931732255320154
Bundle size diffs [🚀 Bundle size reduced!]
  • background: 0 Bytes (0.00%)
  • ui: 1.03 KiB (0.01%)
  • common: -4.81 KiB (-0.06%)

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.

Allow background prop on Container
2 participants