fix(suite-native): pin matrix screen oveflow #17230
Conversation
WalkthroughThis pull request makes adjustments in two separate components. In one component, changes include the addition of a new state variable and event handler that determine whether an image is rendered based on a comparison of the pin matrix layout's height against a computed screen height threshold. The image styling and layout structure have been updated to ensure responsiveness. In the other component, the property that explicitly set the screen’s bottom inset to false was removed, altering how insets are handled during rendering. No public or exported entity declarations were modified in the second component. ✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
suite-native/module-authorize-device/src/components/connect/PinOnKeypad.tsx (2)
89-89: Initialization default could cause layout shift.Setting isImageDisplayed to true by default might cause a flash of the image on initial render before layout measurements are taken, potentially causing a layout shift.
Consider using a null state initially and only showing the image after layout is measured:
-const [isImageDisplayed, setIsImageDisplayed] = useState(true); +const [isImageDisplayed, setIsImageDisplayed] = useState<boolean | null>(null);Then in the render:
-{isImageDisplayed && ( +{isImageDisplayed !== false && (
93-101: Document the threshold selection rationale.The current implementation uses 50% of screen height as the threshold, but there's no clear explanation for why this specific value was chosen.
Consider extracting this magic number to a named constant and documenting the rationale:
-const SCREEN_HEIGHT = getScreenHeight(); +// We hide the device image if the pin matrix takes up more than half the screen +// to prevent overflow issues on smaller devices +const SCREEN_HEIGHT = getScreenHeight(); +const PIN_MATRIX_HEIGHT_THRESHOLD = 0.5 * SCREEN_HEIGHT; // And then in the handler: -if (event.nativeEvent.layout.height > 0.5 * SCREEN_HEIGHT) { +if (event.nativeEvent.layout.height > PIN_MATRIX_HEIGHT_THRESHOLD) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
suite-native/module-authorize-device/src/components/connect/PinOnKeypad.tsx(3 hunks)suite-native/module-device-settings/src/components/DeviceInteractionScreenWrapper.tsx(0 hunks)
💤 Files with no reviewable changes (1)
- suite-native/module-device-settings/src/components/DeviceInteractionScreenWrapper.tsx
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: EAS Update
- GitHub Check: Analyze with CodeQL (javascript)
- GitHub Check: prepare_android_test_app
- GitHub Check: Setup and Cache Dependencies
🔇 Additional comments (4)
suite-native/module-authorize-device/src/components/connect/PinOnKeypad.tsx (4)
1-8: Appropriate imports added for new functionality.The added imports for useState, LayoutChangeEvent, and getScreenHeight provide the necessary tools to implement the responsive behavior needed to address the pin matrix overflow issue.
72-76: Good image styling approach for responsive display.The image styling with flex: 1, width: 100%, and maxHeight: 194 creates a responsive image that maintains proportions while setting appropriate constraints.
104-117: Clean responsive layout implementation.The responsive approach to conditionally rendering the image based on available space is well implemented. The flex justification and alignment properties ensure proper content placement when the image is hidden.
118-149: Effective event handling implementation.Using the onLayout event handler on the Box wrapper is an effective way to measure the pin matrix and make layout decisions. The approach correctly implements the PR objective to prevent overflow by conditionally displaying the image.
| maxHeight: 194, | ||
| })); | ||
|
|
||
| const SCREEN_HEIGHT = getScreenHeight(); |
There was a problem hiding this comment.
💡 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 tsLength 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 tsLength 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.addEventListeneror equivalent) to update the screen height dynamically.
Please review and adjust the implementation to ensure that the component correctly reflects display changes after rotation.
|
🚀 Expo preview is ready!
|
| if (event.nativeEvent.layout.height > 0.5 * SCREEN_HEIGHT) { | ||
| setIsImageDisplayed(false); | ||
|
|
||
| return; | ||
| } | ||
| setIsImageDisplayed(true); |
There was a problem hiding this comment.
What about this?
| if (event.nativeEvent.layout.height > 0.5 * SCREEN_HEIGHT) { | |
| setIsImageDisplayed(false); | |
| return; | |
| } | |
| setIsImageDisplayed(true); | |
| setIsImageDisplayed(event.nativeEvent.layout.height <= 0.5 * SCREEN_HEIGHT); |
Related Issue
Resolve #17035