Skip to content

Commit

Permalink
refactor: Replace redesign confirmation BottomModal with BottomSheet (#…
Browse files Browse the repository at this point in the history
…13268)

<!--
Please submit this PR as a draft initially.
Do not mark it as "Ready for review" until the template has been
completely filled out, and PR status checks have passed at least once.
-->


## **Description**

Updates:
- Replaces BottomModal with BottomSheet in redesign Confirm page
- Remove min height to prevent redesign Confirm page from showing excess
space
- Allow BottomSheet styles (rounded corners, footer bottom styles, and
device padding detection padding) rather than custom BottomModal styles
- Update BlockaidBanner to remove surrounding margin to allow for better
reusability
- Replace BlockaidBanner instances margin overrides with surrounding
container margins
- Update BottomSheet to pass style to BottomSheetDialog sheet cc:
@brianacnguyen
- Note: I think we don't need the additional background colors for
header and footer since the parent, BottomSheet animated view sets the
background color. Once I removed them, it triggered many snapshot tests
and code owner reviews from 5 additional teams. Instead of making this
change, we will continue to update the bottomsheet, footer, and header
when applicable

Fixes:
- Fix TransactionReview BlockaidBanner negative padding → turn to
positive padding to allow space between next element
 - Fix Confirm.test console.errors
 
## **Related issues**

Fixes: #13267
Todo follow-up - Related Issue:
#12656

## **Manual testing steps**

Test BottomSheets and BlockaidBanner alerts work as expected. Test
redesign confirmation page works as expected

## **Screenshots/Recordings**

<!-- If applicable, add screenshots and/or recordings to visualize the
before and after of your change. -->

### Before using BottomModal and no scroll behavior


https://github.com/user-attachments/assets/0d4c5453-5824-47fd-8aa5-7405cf19de53

### After using BottomSheet with scroll behavior 

<img width="320"
src="https://github.com/user-attachments/assets/a908f81f-1a28-4e8c-966a-a390430dc7d9">


https://github.com/user-attachments/assets/8f8e2344-7706-49a4-b773-a5226b7826d4

### Before 70% min height

<img width="320"
src="https://github.com/user-attachments/assets/94509d26-5e20-414d-942f-56938620f90a">

### After remove  70% min height 

<img width="320"
src="https://github.com/user-attachments/assets/641022c5-6f23-47a5-9f00-4e9358a96d3b">

### **Before Transaction Review BlockaidBanner negative padding**

<img width="320"
src="https://github.com/user-attachments/assets/c057c4e7-c7a7-42f2-8251-b4f2c075a802">

### **After Transaction Review BlockaidBanner positive padding **

<img width="320"
src="https://github.com/user-attachments/assets/de1ecadb-c7ef-49e1-8ae2-6ce1b5d16997">

### Before Confirm.test console errors

![CleanShot 2025-02-09 at 01 35
46@2x](https://github.com/user-attachments/assets/2653a073-b0de-4c2d-bcea-dfbc11d38246)
![CleanShot 2025-02-09 at 01 35
54@2x](https://github.com/user-attachments/assets/f9ce410c-42c5-4738-b99f-f871456eb82f)

### After Confirm.test no console.errors

![CleanShot 2025-02-09 at 01 49
47@2x](https://github.com/user-attachments/assets/d3bf6580-553d-4b14-96ab-3f529d17db67)


## **Pre-merge author checklist**

- [ ] I’ve followed [MetaMask Contributor
Docs](https://github.com/MetaMask/contributor-docs) and [MetaMask Mobile
Coding
Standards](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/CODING_GUIDELINES.md).
- [ ] I've completed the PR template to the best of my ability
- [ ] I’ve included tests if applicable
- [ ] I’ve documented my code using [JSDoc](https://jsdoc.app/) format
if applicable
- [ ] I’ve applied the right labels on the PR (see [labeling
guidelines](https://github.com/MetaMask/metamask-mobile/blob/main/.github/guidelines/LABELING_GUIDELINES.md)).
Not required for external contributors.

## **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.

---------

Co-authored-by: Brian Nguyen <[email protected]>
  • Loading branch information
digiwand and brianacnguyen authored Feb 28, 2025
1 parent dcf67aa commit a13b9af
Show file tree
Hide file tree
Showing 22 changed files with 187 additions and 135 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@ const BottomSheet = forwardRef<BottomSheetRef, BottomSheetProps>(
children,
onClose,
onOpen,
style,
isInteractable = true,
shouldNavigateBack = true,
isFullscreen = false,
Expand Down Expand Up @@ -107,6 +108,7 @@ const BottomSheet = forwardRef<BottomSheetRef, BottomSheetProps>(
onOpen={onOpenCB}
ref={bottomSheetDialogRef}
isFullscreen={isFullscreen}
style={style}
>
{children}
</BottomSheetDialog>
Expand Down
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
// Third party dependencies.
// eslint-disable-next-line @typescript-eslint/no-shadow
import { StyleSheet, ViewStyle } from 'react-native';

// External dependencies.
Expand All @@ -21,7 +22,7 @@ const styleSheet = (params: {
}) => {
const { vars, theme } = params;
const { colors, shadows } = theme;
const { maxSheetHeight, screenBottomPadding, isFullscreen } = vars;
const { isFullscreen, maxSheetHeight, screenBottomPadding, style } = vars;

return StyleSheet.create({
base: Object.assign({
Expand All @@ -30,18 +31,21 @@ const styleSheet = (params: {
left: 0,
right: 0,
} as ViewStyle) as ViewStyle,
sheet: {
backgroundColor: colors.background.default,
borderTopLeftRadius: 8,
borderTopRightRadius: 8,
maxHeight: maxSheetHeight,
overflow: 'hidden',
paddingBottom: screenBottomPadding,
borderWidth: 1,
borderColor: colors.border.muted,
...(isFullscreen && { height: maxSheetHeight }),
...shadows.size.lg,
},
sheet: Object.assign(
{
backgroundColor: colors.background.default,
borderTopLeftRadius: 8,
borderTopRightRadius: 8,
maxHeight: maxSheetHeight,
overflow: 'hidden',
paddingBottom: screenBottomPadding,
borderWidth: 1,
borderColor: colors.border.muted,
...(isFullscreen && { height: maxSheetHeight }),
...shadows.size.lg,
},
style,
) as ViewStyle,
notchWrapper: {
alignSelf: 'stretch',
padding: 4,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ const BottomSheetDialog = forwardRef<
isInteractable = true,
onClose,
onOpen,
style,
...props
},
ref,
Expand All @@ -71,6 +72,7 @@ const BottomSheetDialog = forwardRef<
const { styles } = useStyles(styleSheet, {
maxSheetHeight,
screenBottomPadding,
style,
isFullscreen,
});
// X and Y values start on top left of the DIALOG
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// Third party dependencies.
import { ViewProps } from 'react-native';
import { StyleProp, ViewProps, ViewStyle } from 'react-native';

/**
* BottomSheetDialog component props.
Expand Down Expand Up @@ -40,5 +40,6 @@ export interface BottomSheetDialogRef {
export interface BottomSheetDialogStyleSheetVars {
maxSheetHeight: number;
screenBottomPadding: number;
style: StyleProp<ViewStyle>;
isFullscreen: boolean;
}
2 changes: 0 additions & 2 deletions app/components/Nav/App/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -1039,12 +1039,10 @@ const App = (props) => {
<Stack.Screen
name={Routes.CONFIRM_FLAT_PAGE}
component={ConfirmRequest}
options={{ animationEnabled: true }}
/>
<Stack.Screen
name={Routes.CONFIRM_MODAL}
component={ConfirmDappRequest}
options={{ animationEnabled: true }}
/>
</Stack.Navigator>
</NavigationContainer>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -595,7 +595,7 @@ exports[`Approval render matches snapshot 1`] = `
<View
style={
{
"marginBottom": -8,
"marginBottom": 8,
"marginHorizontal": 16,
}
}
Expand Down
24 changes: 7 additions & 17 deletions app/components/Views/confirmations/Confirm/Confirm.styles.ts
Original file line number Diff line number Diff line change
@@ -1,12 +1,15 @@
import { StyleSheet } from 'react-native';

import Device from '../../../../util/device';
import { Theme } from '../../../../util/theme/models';

const styleSheet = (params: { theme: Theme }) => {
const styleSheet = (params: {
theme: Theme;
}) => {
const { theme } = params;

return StyleSheet.create({
bottomSheetDialogSheet: {
backgroundColor: theme.colors.background.alternative,
},
flatContainer: {
position: 'absolute',
top: 0,
Expand All @@ -16,22 +19,9 @@ const styleSheet = (params: { theme: Theme }) => {
zIndex: 9999,
backgroundColor: theme.colors.background.alternative,
justifyContent: 'space-between',
paddingHorizontal: 16,
},
modalContainer: {
backgroundColor: theme.colors.background.alternative,
scrollView: {
paddingHorizontal: 16,
paddingVertical: 24,
borderTopLeftRadius: 20,
borderTopRightRadius: 20,
paddingBottom: Device.isIphoneX() ? 20 : 0,
height: '85%',
},
scrollableSection: {
padding: 4,
},
scrollable: {
height: '75%',
},
});
};
Expand Down
80 changes: 66 additions & 14 deletions app/components/Views/confirmations/Confirm/Confirm.test.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,6 @@
import React from 'react';
import { SafeAreaProvider } from 'react-native-safe-area-context';
import { act } from '@testing-library/react-native';

import renderWithProvider from '../../../../util/test/renderWithProvider';
import {
Expand All @@ -12,6 +14,40 @@ import * as ConfirmationRedesignEnabled from '../hooks/useConfirmationRedesignEn

import { Confirm } from './Confirm';

jest.mock('@react-navigation/native', () => ({
...jest.requireActual('@react-navigation/native'),
useNavigation: () => ({
addListener: jest.fn(),
dispatch: jest.fn(),
goBack: jest.fn(),
navigate: jest.fn(),
removeListener: jest.fn(),
}),
}));

jest.mock('react-native/Libraries/Linking/Linking', () => ({
addEventListener: jest.fn(),
removeEventListener: jest.fn(),
openURL: jest.fn(),
canOpenURL: jest.fn(),
getInitialURL: jest.fn(),
}));

jest.mock('react-native-safe-area-context', () => {
const inset = { top: 0, right: 0, bottom: 0, left: 0 };
const frame = { width: 0, height: 0, x: 0, y: 0 };

return {
...jest.requireActual('react-native-safe-area-context'),
SafeAreaProvider: jest.fn().mockImplementation(({ children }) => children),
SafeAreaConsumer: jest
.fn()
.mockImplementation(({ children }) => children(inset)),
useSafeAreaInsets: jest.fn().mockImplementation(() => inset),
useSafeAreaFrame: jest.fn().mockImplementation(() => frame),
};
});

jest.mock('../../../../core/Engine', () => ({
getTotalFiatAccountBalance: () => ({ tokenFiat: 10 }),
context: {
Expand All @@ -31,14 +67,10 @@ jest.mock('../../../../core/Engine', () => ({
},
controllerMessenger: {
subscribe: jest.fn(),
unsubscribe: jest.fn(),
},
}));

jest.mock('../../../../util/address', () => ({
...jest.requireActual('../../../../util/address'),
getAddressAccountType: (str: string) => str,
}));

jest.mock('react-native-gzip', () => ({
deflate: (str: string) => str,
}));
Expand All @@ -61,10 +93,22 @@ describe('Confirm', () => {
expect(getByTestId('modal-confirmation-container')).toBeDefined();
});

it('renders correct information for personal sign', async () => {
const { getAllByRole, getByText } = renderWithProvider(<Confirm />, {
state: personalSignatureConfirmationState,
it('renders a flat confirmation for specified type(s): staking deposit', () => {
const { getByTestId } = renderWithProvider(<Confirm />, {
state: stakingDepositConfirmationState,
});
expect(getByTestId('flat-confirmation-container')).toBeDefined();
});

it('renders correct information for personal sign', () => {
const { getAllByRole, getByText } = renderWithProvider(
<SafeAreaProvider>
<Confirm />
</SafeAreaProvider>,
{
state: personalSignatureConfirmationState,
},
);
expect(getByText('Signature request')).toBeDefined();
expect(
getByText('Review request details before you confirm.'),
Expand All @@ -76,11 +120,16 @@ describe('Confirm', () => {
expect(getAllByRole('button')).toHaveLength(2);
});

it('renders correct information for typed sign v1', async () => {
it('renders correct information for typed sign v1', () => {
const { getAllByRole, getAllByText, getByText, queryByText } =
renderWithProvider(<Confirm />, {
state: typedSignV1ConfirmationState,
});
renderWithProvider(
<SafeAreaProvider>
<Confirm />
</SafeAreaProvider>,
{
state: typedSignV1ConfirmationState,
},
);
expect(getByText('Signature request')).toBeDefined();
expect(getByText('Request from')).toBeDefined();
expect(getByText('metamask.github.io')).toBeDefined();
Expand All @@ -102,18 +151,21 @@ describe('Confirm', () => {
expect(getByText('Advanced details')).toBeDefined();
});

it('renders blockaid banner if confirmation has blockaid error response', async () => {
it('renders a blockaid banner if the confirmation has blockaid error response', async () => {
const { getByText } = renderWithProvider(<Confirm />, {
state: {
...typedSignV1ConfirmationState,
signatureRequest: { securityAlertResponse },
},
});

await act(async () => undefined);

expect(getByText('Signature request')).toBeDefined();
expect(getByText('This is a deceptive request')).toBeDefined();
});

it('returns null if re-design is not enabled for confirmation', () => {
it('returns null if confirmation redesign is not enabled', () => {
jest
.spyOn(ConfirmationRedesignEnabled, 'useConfirmationRedesignEnabled')
.mockReturnValue({ isRedesignedEnabled: false });
Expand Down
31 changes: 18 additions & 13 deletions app/components/Views/confirmations/Confirm/Confirm.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -6,34 +6,36 @@ import {
View,
} from 'react-native';

import BottomSheet from '../../../../component-library/components/BottomSheets/BottomSheet';
import { useStyles } from '../../../../component-library/hooks';
import BottomModal from '../components/UI/BottomModal';
import Footer from '../components/Confirm/Footer';
import { Footer } from '../components/Confirm/Footer';
import Info from '../components/Confirm/Info';
import { LedgerContextProvider } from '../context/LedgerContext';
import { QRHardwareContextProvider } from '../context/QRHardwareContext/QRHardwareContext';
import SignatureBlockaidBanner from '../components/Confirm/SignatureBlockaidBanner';
import Title from '../components/Confirm/Title';
import useApprovalRequest from '../hooks/useApprovalRequest';
import { useConfirmActions } from '../hooks/useConfirmActions';
import { useConfirmationRedesignEnabled } from '../hooks/useConfirmationRedesignEnabled';
import { useFlatConfirmation } from '../hooks/useFlatConfirmation';
import useApprovalRequest from '../hooks/useApprovalRequest';
import { useConfirmActions } from '../hooks/useConfirmActions';
import styleSheet from './Confirm.styles';

const ConfirmWrapped = ({
styles,
testID,
}: {
styles: StyleSheet.NamedStyles<Record<string, unknown>>;
testID?: string;
}) => (
<QRHardwareContextProvider>
<LedgerContextProvider>
<Title />
<ScrollView style={styles.scrollable}>
<TouchableWithoutFeedback>
<View style={styles.scrollableSection}>
<ScrollView style={styles.scrollView}>
<TouchableWithoutFeedback testID={testID}>
<>
<SignatureBlockaidBanner />
<Info />
</View>
</>
</TouchableWithoutFeedback>
</ScrollView>
<Footer />
Expand Down Expand Up @@ -62,10 +64,13 @@ export const Confirm = () => {
}

return (
<BottomModal onClose={onReject} testID="modal-confirmation-container">
<View style={styles.modalContainer} testID={approvalRequest?.type}>
<ConfirmWrapped styles={styles} />
</View>
</BottomModal>
<BottomSheet
isInteractable={false}
onClose={onReject}
style={styles.bottomSheetDialogSheet}
testID="modal-confirmation-container"
>
<ConfirmWrapped testID={approvalRequest?.type} styles={styles} />
</BottomSheet>
);
};
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,7 @@ const createStyles = (colors: any) =>
textDecorationLine: 'underline',
...fontStyles.bold,
},
blockaidBanner: {
blockaidBannerContainer: {
marginBottom: 10,
marginTop: 20,
marginHorizontal: 10,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -167,7 +167,7 @@ const createStyles = (colors: any) =>
justifyContent: 'center',
alignItems: 'center',
},
blockaidWarning: {
blockaidBannerContainer: {
marginBottom: 10,
marginTop: 20,
marginHorizontal: 10,
Expand Down
Loading

0 comments on commit a13b9af

Please sign in to comment.