feat(pro): redesign Pro detail screen and settings banner#371
Conversation
- Rebuild ProDetailScreen with hero section, dark promo banner, integration grid, and CTA - Add persistent dismissible Pro banner to top of SettingsScreen - Add Pro nav button (zap icon + PRO badge) in settings list - Persist banner dismissed state to AsyncStorage via appStore
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request redesigns the Pro Detail screen with a new hero section, promo banner, and integrations grid, while also introducing a dismissible PRO banner to the Settings screen. The appStore was updated to persist the banner's dismissal state. Feedback focuses on restoring the missing back button on the Pro Detail screen, avoiding hardcoded hex colors and percentage-based layout hacks, and removing unused style definitions. Additionally, it is recommended to use a more flexible grid layout for integrations and to increase the tap target size for the banner's dismiss button to improve usability.
| <View style={styles.header}> | ||
| <View style={styles.logoRow}> | ||
| <View style={styles.logoGrid}> | ||
| <View style={styles.logoDotRow}> | ||
| <View style={styles.logoDot} /> | ||
| <View style={styles.logoDot} /> | ||
| </View> | ||
| <View style={styles.featureText}> | ||
| <Text style={styles.featureTitle}>{f.title}</Text> | ||
| <Text style={styles.featureDesc}>{f.desc}</Text> | ||
| <View style={styles.logoDotRow}> | ||
| <View style={styles.logoDot} /> | ||
| <View style={styles.logoDot} /> | ||
| </View> | ||
| </View> | ||
| ))} | ||
| <Text style={styles.logoText}>Off Grid Pro</Text> | ||
| </View> | ||
| <TouchableOpacity style={styles.getProButton} onPress={handleCTA}> | ||
| <Text style={styles.getProButtonText}>Get Pro</Text> | ||
| </TouchableOpacity> | ||
| </View> |
There was a problem hiding this comment.
The redesigned header is missing a back button. Since this screen is part of a navigation stack, users need an explicit way to return to the previous screen (Settings). The previous implementation included an arrow-left icon for this purpose.
There was a problem hiding this comment.
The header intentionally keeps the branded logo + 'Get Pro' CTA as a marketing element on this screen — it's a landing/paywall screen, not a standard nav screen. Back navigation is handled by the iOS swipe gesture and Android hardware back button. The screen is presented with slide_from_bottom animation and headerShown: false in the navigator.
| {INTEGRATIONS.slice(0, 2).map(item => ( | ||
| <View key={item.title} style={styles.gridCard}> | ||
| <View style={styles.gridIconWrap}> | ||
| <Icon name={item.icon} size={20} color={colors.textSecondary} /> | ||
| </View> | ||
| <Text style={styles.gridCardTitle}>{item.title}</Text> | ||
| <Text style={styles.gridCardDesc}>{item.desc}</Text> | ||
| </View> | ||
| ))} | ||
| </View> | ||
|
|
||
| <View style={styles.gridRow}> | ||
| {INTEGRATIONS.slice(2, 4).map(item => ( | ||
| <View key={item.title} style={styles.gridCard}> | ||
| <View style={styles.gridIconWrap}> | ||
| <Icon name={item.icon} size={20} color={colors.textSecondary} /> | ||
| </View> | ||
| <Text style={styles.gridCardTitle}>{item.title}</Text> | ||
| <Text style={styles.gridCardDesc}>{item.desc}</Text> | ||
| </View> | ||
| ))} | ||
| </View> |
There was a problem hiding this comment.
Manually slicing the INTEGRATIONS array into fixed rows is fragile. If the number of integrations changes, this logic will either miss items or render empty rows. Consider using a more flexible layout approach, such as a container with flexDirection: 'row' and flexWrap: 'wrap'. Avoid creating a shared utility for this logic unless it is used in more than two places to prevent premature abstraction.
References
- Avoid premature abstraction. A shared utility for duplicated logic should be considered only when the logic is used in more than two places.
There was a problem hiding this comment.
The INTEGRATIONS array is intentionally fixed at 4 items — these map to the 4 core product features and won't change. Introducing flexWrap or a utility for a static 4-item list would be premature abstraction. If items are added in future, the layout can be revisited then.
| promoBanner: { | ||
| marginHorizontal: SPACING.xl, | ||
| marginBottom: SPACING.xl, | ||
| backgroundColor: '#1C2B22', |
There was a problem hiding this comment.
Avoid hardcoding hex colors like #1C2B22. These should be defined in the theme configuration to ensure consistency and support for potential future theme changes.
There was a problem hiding this comment.
Acknowledged. The dark green (#1C2B22 / gradient stops) is a one-off branded surface for this marketing screen and doesn't belong in the general theme palette. Adding a proSurface token to the theme would be the right fix but is out of scope for this PR. Tracked for a follow-up.
| footer: { | ||
| paddingHorizontal: SPACING.xl, | ||
| paddingTop: SPACING.xl, | ||
| borderTopWidth: 1, | ||
| borderTopColor: colors.border, | ||
| alignItems: 'center' as const, | ||
| gap: SPACING.md, | ||
| }, | ||
| footerBrand: { | ||
| ...TYPOGRAPHY.body, | ||
| color: colors.text, | ||
| }, | ||
| footerLinks: { | ||
| flexDirection: 'row' as const, | ||
| gap: SPACING.lg, | ||
| flexWrap: 'wrap' as const, | ||
| justifyContent: 'center' as const, | ||
| }, | ||
| footerLink: { | ||
| ...TYPOGRAPHY.bodySmall, | ||
| color: colors.textSecondary, | ||
| }, | ||
| footerCopy: { | ||
| ...TYPOGRAPHY.meta, | ||
| color: colors.textMuted, | ||
| textAlign: 'center' as const, | ||
| }, |
There was a problem hiding this comment.
The footer styles and its sub-properties are defined but never used in the component's render method. Please either implement the footer UI or remove the dead style definitions.
There was a problem hiding this comment.
Fixed — dead footer styles have been removed in a follow-up commit.
| <TouchableOpacity onPress={() => setProBannerDismissed(true)} style={styles.proCloseButton}> | ||
| <Icon name="x" size={16} color={colors.textMuted} /> | ||
| </TouchableOpacity> |
There was a problem hiding this comment.
The dismiss button for the Pro banner has a very small tap target. Interactive elements should have a minimum tap target size of at least 36px to improve usability on touch devices.
There was a problem hiding this comment.
Acknowledged. The tap target is small but acceptable for a secondary dismiss action on a banner. Will revisit if flagged in user testing.
| flexDirection: 'row' as const, | ||
| alignItems: 'center' as const, | ||
| gap: SPACING.sm, | ||
| width: '48%' as const, |
There was a problem hiding this comment.
Using a hardcoded percentage width (48%) for grid items is a layout hack that can lead to inconsistencies across different screen sizes. It is better to use flex: 1 on the items within a row or leverage gap with flexWrap on the parent container.
There was a problem hiding this comment.
Fixed — replaced the 48% width hack with an explicit row-based layout using flex: 1 on each item, so the grid scales correctly across all screen sizes.
Codecov Report❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (50.00%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #371 +/- ##
==========================================
- Coverage 82.85% 82.83% -0.03%
==========================================
Files 235 235
Lines 12017 12027 +10
Branches 3283 3286 +3
==========================================
+ Hits 9957 9962 +5
- Misses 1203 1207 +4
- Partials 857 858 +1
🚀 New features to boost your workflow:
|
LinearGradient on iOS does not constrain child width like a View, causing the banner subtitle to render on one line and overflow the screen, and leaving a white gap below the MCP card. - Wrap banner and MCP card contents in a View, with LinearGradient as an absolute-filled background - Drop Menlo font from banner title/subtitle (system font is narrower and renders consistently across iOS/Android) - Remove unused useWindowDimensions / bannerWidth calculation
|
1 participant
Summary
Type of Change
Screenshots / Screen Recordings
Android
iOS
Checklist
General
Testing
npm test)React Native Specific
project.pbxproj)SPACING/TYPOGRAPHYconstants from the themeuseThemedStylespattern (not inline or staticStyleSheet.create)FlatList/FlashList(not.map()insideScrollView)Performance & Models
/vs\\)Security
Related Issues
Additional Notes