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.

Sign up for GitHub

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

fix(suite-native): pin matrix screen oveflow #17230

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from
+62 −31
Open

fix(suite-native): pin matrix screen oveflow #17230

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3a9d70f
fix(suite-native): pin matrix screen oveflow
PeKne Feb 25, 2025
11453ba
fixup! fix(suite-native): pin matrix screen oveflow
PeKne Feb 27, 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
92 changes: 62 additions & 30 deletions suite-native/module-authorize-device/src/components/connect/PinOnKeypad.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,11 @@
import { useState } from 'react';
import { LayoutChangeEvent } from 'react-native';

import { PinFormValues, pinFormSchema } from '@suite-common/validators';
import { Box, Card, HStack, Image, Loader, Text, VStack } from '@suite-native/atoms';
import { Form, useForm } from '@suite-native/forms';
import { Translation, TxKeyPath } from '@suite-native/intl';
import { getScreenHeight } from '@trezor/env-utils';
import { prepareNativeStyle, useNativeStyles } from '@trezor/styles';

import { PinFormControlButtons } from './PinFormControlButtons';
Expand Down Expand Up @@ -65,6 +69,14 @@ const loaderWrapperStyle = prepareNativeStyle(utils => ({
backgroundColor: utils.colors.backgroundSurfaceElevation1,
}));

const imageStyle = prepareNativeStyle(() => ({
flex: 1,
width: '100%',
maxHeight: 194,
}));

const SCREEN_HEIGHT = getScreenHeight();
Copy link

Choose a reason for hiding this comment

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

💡 Verification agent

🧩 Analysis chain

Consider handling orientation changes for SCREEN_HEIGHT.

The SCREEN_HEIGHT constant is defined outside the component and won't update if orientation changes. If the app supports rotation, this could lead to incorrect display decisions.

🏁 Script executed:

#!/bin/bash
# Check if the app handles orientation changes elsewhere
rg -A 2 -B 2 "orientation|Orientation|portrait|landscape" --type ts

Length of output: 20662

🏁 Script executed:

#!/bin/bash
# Search for usage of getScreenHeight in the repository to see if there's any dynamic updating
rg "getScreenHeight" --type ts

Length of output: 2261

Action required: Dynamically update SCREEN_HEIGHT on orientation change

The current implementation of

const SCREEN_HEIGHT = getScreenHeight();

defines SCREEN_HEIGHT at module load time, meaning its value won’t update when the device’s orientation changes. Given that orientation events are handled elsewhere in the app (e.g., via the orientationchange listener in SuiteLayout), you should consider one of the following approaches in this component:

  • Move the call to getScreenHeight() inside the component’s render or a hook (such as using state with an event listener on orientation change).
  • Subscribe to orientation or resize updates (using React Native’s Dimensions.addEventListener or equivalent) to update the screen height dynamically.

Please review and adjust the implementation to ensure that the component correctly reflects display changes after rotation.


export const PinOnKeypad = ({ variant, onSuccess }: PinOnKeypadProps) => {
const { applyStyle } = useNativeStyles();
const form = useForm<PinFormValues>({
Expand All @@ -74,41 +86,61 @@ export const PinOnKeypad = ({ variant, onSuccess }: PinOnKeypadProps) => {
},
});

const [isImageDisplayed, setIsImageDisplayed] = useState(true);

const translations = translationsMap[variant];

// Hide image if the screen is too small to fit both the image and the pin matrix.
const handlePinMatrixLayoutEvent = (event: LayoutChangeEvent) =>
setIsImageDisplayed(event.nativeEvent.layout.height <= 0.5 * SCREEN_HEIGHT);

Comment on lines +93 to +96
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

Logic seems inverted in handlePinMatrixLayoutEvent

The current condition hides the image when there should be plenty of space (when layout height is small). This seems counterintuitive.

-    const handlePinMatrixLayoutEvent = (event: LayoutChangeEvent) =>
-        setIsImageDisplayed(event.nativeEvent.layout.height <= 0.5 * SCREEN_HEIGHT);
+    const handlePinMatrixLayoutEvent = (event: LayoutChangeEvent) =>
+        setIsImageDisplayed(event.nativeEvent.layout.height < 0.5 * screenHeight);

The exact condition might need further refinement based on your specific layout requirements. Consider this alternative that was suggested in a previous comment:

setIsImageDisplayed(event.nativeEvent.layout.height <= 0.5 * SCREEN_HEIGHT);

Committable suggestion skipped: line range outside the PR's diff.

return (
<VStack spacing="sp16" alignItems="center" flex={1} marginTop="sp24">
<Image source={deviceImageMap.T1B1} width={161} height={194} />
<Form form={form}>
<VStack spacing="sp8" alignItems="center">
<Text color="textSubdued">
<Translation id="moduleConnectDevice.pinScreen.form.keypadInfo" />
</Text>
<Box style={applyStyle(pinProgressWrapperStyle)}>
<PinFormProgress title={<Translation id={translations.formTitle} />} />
</Box>
</VStack>
<Card style={applyStyle(cardStyle)}>
<VStack justifyContent="center" alignItems="center" spacing="sp24">
{pinMatrix.map(pinRow => (
<HStack key={pinRow.join('')} spacing="sp16">
{pinRow.map(value => (
<PinMatrixButton key={value} value={value} />
))}
</HStack>
))}
<VStack
spacing="sp16"
alignItems="center"
flex={1}
justifyContent="flex-end"
marginTop="sp24"
>
{isImageDisplayed && (
<Image
source={deviceImageMap.T1B1}
style={applyStyle(imageStyle)}
contentFit="contain"
/>
)}
<Box onLayout={handlePinMatrixLayoutEvent}>
<Form form={form}>
<VStack spacing="sp8" alignItems="center">
<Text color="textSubdued" textAlign="center">
<Translation id="moduleConnectDevice.pinScreen.form.keypadInfo" />
</Text>
<Box style={applyStyle(pinProgressWrapperStyle)}>
<PinFormProgress title={<Translation id={translations.formTitle} />} />
</Box>
</VStack>
<PinFormControlButtons onSuccess={onSuccess} />
{form.formState.isSubmitted && (
<VStack style={applyStyle(loaderWrapperStyle)} spacing="sp16">
<Loader size="large" />
<Text variant="titleSmall">
<Translation id={translations.loaderText} />
</Text>
<Card style={applyStyle(cardStyle)}>
<VStack justifyContent="center" alignItems="center" spacing="sp24">
{pinMatrix.map(pinRow => (
<HStack key={pinRow.join('')} spacing="sp16">
{pinRow.map(value => (
<PinMatrixButton key={value} value={value} />
))}
</HStack>
))}
</VStack>
)}
</Card>
</Form>
<PinFormControlButtons onSuccess={onSuccess} />
{form.formState.isSubmitted && (
<VStack style={applyStyle(loaderWrapperStyle)} spacing="sp16">
<Loader size="large" />
<Text variant="titleSmall">
<Translation id={translations.loaderText} />
</Text>
</VStack>
)}
</Card>
</Form>
</Box>
</VStack>
);
};
1 change: 0 additions & 1 deletion suite-native/module-device-settings/src/components/DeviceInteractionScreenWrapper.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,6 @@ export const DeviceInteractionScreenWrapper = ({
return (
<Screen
header={<ScreenHeader closeActionType="close" closeAction={closeAction} />}
hasBottomInset={false}
isScrollable={false}
>
{children}
Expand Down
Loading