Skip to content

fix: alignment of Snaps icons within ButtonLink/SnapUIButton component #14243

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

Open
wants to merge 30 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
147edd5
changed SnapUIButton component to a custom component that works with …
Daniel-Cross Mar 26, 2025
9d280d9
fixed lint issues
Daniel-Cross Mar 26, 2025
252dee3
updated snapshots
Daniel-Cross Mar 26, 2025
bde49a6
Merge branch 'main' into 3244-alignment-is-off-on-some-snapuibuttons
Daniel-Cross Mar 26, 2025
bdbd6b8
Merge branch 'main' into 3244-alignment-is-off-on-some-snapuibuttons
Daniel-Cross Mar 27, 2025
48bbe09
added tests
Daniel-Cross Mar 27, 2025
01b93a3
fixed merge conflicts
Daniel-Cross Mar 28, 2025
5862765
made changes to address the alignment and colour inheritance
Daniel-Cross Mar 28, 2025
0718eeb
Merge branch 'main' into 3244-alignment-is-off-on-some-snapuibuttons
Daniel-Cross Mar 28, 2025
dc24a00
updated types
Daniel-Cross Mar 28, 2025
3289ecd
changed type to interface
Daniel-Cross Mar 28, 2025
2ef9b13
removed log and changed any
Daniel-Cross Mar 28, 2025
cf494ed
snapshot update
Daniel-Cross Mar 31, 2025
795ab2a
Merge branch 'main' into 3244-alignment-is-off-on-some-snapuibuttons
Daniel-Cross Mar 31, 2025
8b8c459
reverted comment
Daniel-Cross Mar 31, 2025
5779a82
removed centre alignment
Daniel-Cross Mar 31, 2025
6628758
updated snapshot
Daniel-Cross Mar 31, 2025
87af066
revert button while fix
Daniel-Cross Mar 31, 2025
afe910e
updated tests to support new structure
Daniel-Cross Mar 31, 2025
fdbfaa4
Merge branch 'main' into 3244-alignment-is-off-on-some-snapuibuttons
Daniel-Cross Mar 31, 2025
c29d582
removed colours from snapuibutton as it's now been moved to button.ts
Daniel-Cross Mar 31, 2025
39fce31
addressed lint issues in test
Daniel-Cross Mar 31, 2025
9fcc492
linting on tests
Daniel-Cross Mar 31, 2025
6cd0137
linting issue
Daniel-Cross Mar 31, 2025
97b23b5
snapshot update
Daniel-Cross Mar 31, 2025
e1b84cc
Merge branch 'main' into 3244-alignment-is-off-on-some-snapuibuttons
Daniel-Cross Mar 31, 2025
c76a6d8
added tests
Daniel-Cross Apr 1, 2025
b03005d
linting issue
Daniel-Cross Apr 1, 2025
82004b0
changed type to interface
Daniel-Cross Apr 1, 2025
51e9d20
Merge branch 'main' into 3244-alignment-is-off-on-some-snapuibuttons
Daniel-Cross Apr 2, 2025
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
193 changes: 193 additions & 0 deletions app/components/Snaps/SnapUIButton/SnapUIButton.test.tsx
Original file line number Diff line number Diff line change
@@ -0,0 +1,193 @@
import React from 'react';
import { render, fireEvent } from '@testing-library/react-native';
import { SnapUIButton } from './SnapUIButton';
import { useSnapInterfaceContext } from '../SnapInterfaceContext';
import { ButtonType, UserInputEventType } from '@metamask/snaps-sdk';
import Text from '../../../component-library/components/Texts/Text';
import { View } from 'react-native';
import AnimatedLottieView from 'lottie-react-native';

jest.mock('../SnapInterfaceContext', () => ({
useSnapInterfaceContext: jest.fn(),
}));

jest.mock('../../../util/theme', () => ({
useTheme: jest.fn().mockReturnValue({
colors: {
background: { default: '#FFFFFF' },
border: { muted: '#CCCCCC', default: '#DDDDDD' },
text: { default: '#000000' },
info: { default: '#0376C9' },
error: { default: '#D73A49' },
},
}),
}));

describe('SnapUIButton', () => {
const mockHandleEvent = jest.fn();
const mockOnPress = jest.fn();

beforeEach(() => {
jest.clearAllMocks();
(useSnapInterfaceContext as jest.Mock).mockReturnValue({
handleEvent: mockHandleEvent,
});
});

it('renders correctly with text children', () => {
const { getByText } = render(
<SnapUIButton variant="primary">Test Button</SnapUIButton>,
);

expect(getByText('Test Button')).toBeTruthy();
});

it('renders correctly when disabled', () => {
const { getByText, getByTestId } = render(
<SnapUIButton variant="primary" disabled testID="disabled-button">
Disabled Button
</SnapUIButton>,
);

expect(getByText('Disabled Button')).toBeTruthy();
expect(getByTestId('disabled-button').props.disabled).toBe(true);
});

it('renders with loading state', () => {
const { UNSAFE_getByType } = render(
<SnapUIButton variant="primary" loading>
Loading Button
</SnapUIButton>,
);

expect(UNSAFE_getByType(AnimatedLottieView)).toBeTruthy();
});

it('calls onPress and handles ButtonClickEvent when pressed', () => {
const { getByText } = render(
<SnapUIButton variant="primary" onPress={mockOnPress} name="test-button">
Click Me
</SnapUIButton>,
);

fireEvent.press(getByText('Click Me'));

expect(mockOnPress).toHaveBeenCalledTimes(1);
expect(mockHandleEvent).toHaveBeenCalledWith({
event: UserInputEventType.ButtonClickEvent,
name: 'test-button',
});
});

it('handles FormSubmitEvent when button type is Submit', () => {
const { getByText } = render(
<SnapUIButton
variant="primary"
type={ButtonType.Submit}
name="test-button"
form="test-form"
>
Submit Form
</SnapUIButton>,
);

fireEvent.press(getByText('Submit Form'));

expect(mockHandleEvent).toHaveBeenCalledWith({
event: UserInputEventType.ButtonClickEvent,
name: 'test-button',
});
expect(mockHandleEvent).toHaveBeenCalledWith({
event: UserInputEventType.FormSubmitEvent,
name: 'test-form',
});
});

it('renders icon component correctly without wrapping in Text', () => {
const MockIcon = () => <View testID="mock-icon" />;
MockIcon.displayName = 'Icon';

const { getByTestId } = render(
<SnapUIButton variant="primary" testID="icon-button">
<MockIcon />
</SnapUIButton>,
);

expect(getByTestId('icon-button')).toBeTruthy();
expect(getByTestId('mock-icon')).toBeTruthy();
});

it('renders destructive variant with correct color', () => {
const { UNSAFE_getAllByType } = render(
<SnapUIButton variant="destructive">Destructive Button</SnapUIButton>,
);

const textComponents = UNSAFE_getAllByType(Text);
expect(textComponents.length).toBeGreaterThan(0);
expect(textComponents[0].props.color).toBe('Error');
});

it('renders with custom style', () => {
const customStyle = { backgroundColor: '#FF0000', borderRadius: 16 };
const { getByTestId } = render(
<SnapUIButton
variant="primary"
style={customStyle}
testID="styled-button"
>
Styled Button
</SnapUIButton>,
);

const button = getByTestId('styled-button');
expect(button.props.style.backgroundColor).toBe('#FF0000');
expect(button.props.style.borderRadius).toBe(16);
});

it('handles non-string, non-icon children properly', () => {
const NonStringComponent = () => <View testID="non-string-component" />;

const { getByTestId } = render(
<SnapUIButton variant="primary" testID="custom-children-button">
<NonStringComponent />
</SnapUIButton>,
);

expect(getByTestId('custom-children-button')).toBeTruthy();
expect(getByTestId('non-string-component')).toBeTruthy();
});

it('sets proper accessibility properties', () => {
const { getByTestId } = render(
<SnapUIButton
variant="primary"
testID="accessible-button"
name="button-name"
>
Accessible Button
</SnapUIButton>,
);

const button = getByTestId('accessible-button');
expect(button.props.accessible).toBe(true);
expect(button.props.accessibilityRole).toBe('button');
expect(button.props.accessibilityLabel).toBe('Accessible Button');
});

it('provides fallback to name for accessibilityLabel when children is not a string', () => {
const NonStringComponent = () => <View testID="non-string-component" />;

const { getByTestId } = render(
<SnapUIButton
variant="primary"
testID="accessible-button"
name="button-name"
>
<NonStringComponent />
</SnapUIButton>,
);

const button = getByTestId('accessible-button');
expect(button.props.accessibilityLabel).toBe('button-name');
});
});
149 changes: 118 additions & 31 deletions app/components/Snaps/SnapUIButton/SnapUIButton.tsx
Original file line number Diff line number Diff line change
@@ -1,21 +1,32 @@
import React, { FunctionComponent } from 'react';
import { ButtonType, UserInputEventType } from '@metamask/snaps-sdk';
import ButtonLink from '../../../component-library/components/Buttons/Button/variants/ButtonLink';
import { ButtonLinkProps } from '../../../component-library/components/Buttons/Button/variants/ButtonLink/ButtonLink.types';
import {
TouchableOpacity,
StyleSheet,
ViewStyle,
StyleProp,
TouchableOpacityProps,
} from 'react-native';
import { useSnapInterfaceContext } from '../SnapInterfaceContext';
import Text, {
TextColor,
TextVariant,
} from '../../../component-library/components/Texts/Text';
import AnimatedLottieView from 'lottie-react-native';
import { useTheme } from '../../../util/theme';

export interface SnapUIButtonProps {
export interface SnapUIButtonProps extends TouchableOpacityProps {
name?: string;
loading?: boolean;
type?: ButtonType;
form?: string;
variant: keyof typeof COLORS;
textVariant?: TextVariant;
style?: StyleProp<ViewStyle>;
disabled?: boolean;
onPress?: () => void;
children?: React.ReactNode;
testID?: string;
}

const COLORS = {
Expand All @@ -24,28 +35,31 @@
disabled: TextColor.Muted,
};

export const SnapUIButton: FunctionComponent<
SnapUIButtonProps & ButtonLinkProps
> = ({
export const SnapUIButton: FunctionComponent<SnapUIButtonProps> = ({
name,
children,
form,
type = ButtonType.Button,
variant = 'primary',
disabled = false,
loading = false,
textVariant,
textVariant = TextVariant.BodyMDMedium,
style,
onPress,
Copy link
Member

Choose a reason for hiding this comment

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

?

testID,
...props
}) => {
const { handleEvent } = useSnapInterfaceContext();
const { colors } = useTheme();

const handlePress = () => {
onPress?.();

handleEvent({
event: UserInputEventType.ButtonClickEvent,
name,
});

// Since we don't have onSubmit on mobile, the button submits the form.
Copy link
Member

Choose a reason for hiding this comment

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

Why are we removing this documentation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Re-added this comment

if (type === ButtonType.Submit) {
handleEvent({
event: UserInputEventType.FormSubmitEvent,
Expand All @@ -55,33 +69,106 @@
};

const overriddenVariant = disabled ? 'disabled' : variant;

const color = COLORS[overriddenVariant as keyof typeof COLORS];
Copy link
Member

Choose a reason for hiding this comment

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

We still want to support the colors for the Snap UI button, but we may have to do it at the mapper level once #14355 lands


const isIcon =
React.isValidElement(children) &&
(children.props?.type === 'Icon' ||
(children.type as React.ComponentType<unknown> & { displayName?: string })
Copy link
Member

Choose a reason for hiding this comment

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

What is going on here? What even is .displayName?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah this has been removed, I was experimenting with something that didn't work.

?.displayName === 'Icon');

const styles = StyleSheet.create({
button: {
backgroundColor: colors.background.default,
borderRadius: 8,
paddingVertical: 8,
flexDirection: 'row',
justifyContent: 'center',
alignItems: 'center',
...(style as ViewStyle),
},
content: {
flexDirection: 'row',
alignItems: 'center',
justifyContent: 'center',
},
icon: {
marginRight: 8,
},
lastIcon: {
marginRight: 0,
},
});

const renderContent = () => {
if (loading) {
return (
<AnimatedLottieView
source={{ uri: './loading.json' }}
autoPlay
loop
// eslint-disable-next-line react-native/no-inline-styles
style={{
width: 24,
height: 24,
}}
/>
);
}

if (typeof children === 'string') {
return (
<Text color={color} variant={textVariant}>
{children}
</Text>
);
}

if (React.isValidElement(children) && children.props?.sections) {

Check failure on line 127 in app/components/Snaps/SnapUIButton/SnapUIButton.tsx

View workflow job for this annotation

GitHub Actions / scripts (lint:tsc)

Property 'sections' does not exist on type '{}'.
try {
const modifiedSections = children.props.sections.map((section: any) => {

Check failure on line 129 in app/components/Snaps/SnapUIButton/SnapUIButton.tsx

View workflow job for this annotation

GitHub Actions / scripts (lint:tsc)

Property 'sections' does not exist on type '{}'.
if (section.element === 'RNText') {
return {
...section,
props: {
...section.props,
style: {
...section.props?.style,
color: colors.primary.default,
},
},
};
}
return section;
});

return React.cloneElement(
// eslint-disable-next-line @typescript-eslint/no-explicit-any
children as React.ReactElement<{ sections: any }>,
{
sections: modifiedSections,
},
Copy link
Member

Choose a reason for hiding this comment

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

Seems simpler to try and solve this at the mapper level before we turn it into React components to prevent having to do this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I took a look at moving this over to the button.ts file and it's possible to map the children there correctly instead of in the SnapUIButton.tsx component, however we still have to map the children again in this component to apply the correct colour to the text as we don't have access to the Theme colours, which is a React hook, and not accessible in the button.ts file.

Not really sure how to solve both the alignment issue (multiple Text component wraps) and the text colour inheritance issue at this point if we need this to be in the button.ts mapper component.

I can perhaps create some sort of theme utility that doesn't rely on react, or I can pass them through via params from the utils.ts file via the mapToTemplate function?

Copy link
Member

@FrederikBolding FrederikBolding Mar 31, 2025

Choose a reason for hiding this comment

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

however we still have to map the children again in this component to apply the correct colour to the text as we don't have access to the Theme colours, which is a React hook, and not accessible in the button.ts file.

We already have access to the theme values in the mappers! For reference:

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Awesome, this might work then

);
} catch (error) {
console.log('Error modifying sections:', error);
}
}

return children;
Copy link
Member

Choose a reason for hiding this comment

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

One of the problems with this is that we don't inherit the text color when complex children are used:
Screenshot 2025-03-27 at 17 06 30

Also for some reason this text is now center aligned?

Can be reproed by rendering

<Box>
  <Button>Foo <Icon name="arrow-right" /> Bar</Button>
</Box>

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! Let me take a look and make some changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This seems to be resolved now

};

return (
<ButtonLink
{...props}
id={name}
<TouchableOpacity
style={styles.button}
onPress={handlePress}
disabled={disabled}
label={
loading ? (
<AnimatedLottieView
source={{ uri: './loading.json' }}
autoPlay
loop
// eslint-disable-next-line react-native/no-inline-styles
style={{
width: 24,
height: 24,
}}
/>
) : (
<Text color={color} variant={textVariant}>
{children}
</Text>
)
}
/>
accessible
accessibilityRole="button"
accessibilityLabel={typeof children === 'string' ? children : name}
testID={testID}
{...props}
>
{renderContent()}
</TouchableOpacity>
);
};
Loading
Loading