fix(suite-native): welcome flow ui review fixes #16868
Conversation
WalkthroughThis pull request introduces modifications to two screens within the onboarding module. In the Possibly related PRs
Suggested labels
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 90000ms (4)
🪧 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: 0
🧹 Nitpick comments (2)
suite-native/module-onboarding/src/screens/AnalyticsConsentScreen.tsx (1)
100-119: Add accessibility props to enhance screen reader support.The UI changes improve user interaction by making the entire consent area tappable, with consistent behavior between tapping the area and toggling the switch. Consider enhancing accessibility by adding the following props:
<TouchableOpacity onPress={toggleAnalyticsConsent} activeOpacity={0.5} + accessible={true} + accessibilityRole="switch" + accessibilityState={{ checked: isEnabled }} + accessibilityLabel={<Translation id="moduleOnboarding.analyticsConsentScreen.helpSwitchTitle" />} >suite-native/module-onboarding/src/screens/BiometricsScreen.tsx (1)
99-99: Fix inconsistent test ID.The test ID "@onboarding/UserDataConsent/enableBtn" appears to be copied from a different screen. Consider updating it to match the current screen name.
- testID="@onboarding/UserDataConsent/enableBtn" + testID="@onboarding/Biometrics/enableBtn"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
suite-native/module-onboarding/src/screens/AnalyticsConsentScreen.tsx(3 hunks)suite-native/module-onboarding/src/screens/BiometricsScreen.tsx(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: EAS Update
- GitHub Check: Setup and Cache Dependencies
- GitHub Check: prepare_android_test_app
- GitHub Check: Analyze with CodeQL (javascript)
🔇 Additional comments (7)
suite-native/module-onboarding/src/screens/AnalyticsConsentScreen.tsx (2)
2-2: LGTM!The
TouchableOpacityimport is correctly added to support the enhanced touch interaction.
61-63: LGTM! Well-structured state update.The
toggleAnalyticsConsentfunction correctly uses the functional update pattern and centralizes the toggle logic, improving maintainability.suite-native/module-onboarding/src/screens/BiometricsScreen.tsx (5)
20-25: LGTM! Well-documented styling change.The title style definition is clear and the comment effectively explains the design requirement for reduced letter spacing.
30-30: LGTM! Clean hook usage.The hook destructuring is focused and follows React best practices.
78-78: LGTM! Improved layout centering.The Box component now properly centers its content both vertically and horizontally, which should provide better visual balance.
81-81: LGTM! Good use of design tokens.Using the predefined spacing token "sp40" instead of a numeric value improves maintainability and ensures consistent spacing across the app.
93-93: LGTM! Proper style application.The custom titleStyle is correctly applied to adjust the letter spacing as per the design requirements.
|
🚀 Expo preview is ready!
|
|
/rebase |
|
Start rebasing: https://github.com/trezor/trezor-suite/actions/runs/13179736305 |
trezor-ci
force-pushed
the
fix/native/onboarding-ui-review
branch
from
d379436 to
7f10710
Compare
| <Box | ||
| flexDirection="row" |
There was a problem hiding this comment.
What about using VStack here?
There was a problem hiding this comment.
VStack does not make sense here because the flexDirection="row". Did you mean HStack? Anyway, I don't see any benefit of using Stack component here. Why would you like to use is?
There was a problem hiding this comment.
Sorry, HStack is what I meant. And I believe it makes sense since you don't have to specify direction explicitly.
Description