fix: Android Menu selector #1527
Conversation
Added design system menu Fixed splash screen for dark mode
WalkthroughThis pull request introduces several improvements to the application's design system and configuration. The changes primarily focus on enhancing menu functionality, updating logging configurations, and modifying app configuration. A new Changes
Sequence DiagramsequenceDiagram
participant App
participant Menu
participant Theme
participant HapticFeedback
App->>Menu: Render with actions
Menu->>Theme: Apply themed colors
Menu->>Menu: Process platform-specific actions
Menu->>HapticFeedback: Trigger feedback on press
Menu->>App: Execute selected action
Possibly related PRs
Suggested reviewers
Poem
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 (
|
Performance Comparison ReportSignificant Changes To DurationThere are no entries Meaningless Changes To DurationShow entries
Show details
Render Count ChangesThere are no entries Render IssuesThere are no entries Added ScenariosThere are no entries Removed ScenariosThere are no entries |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (8)
design-system/Menu/Menu.utils.tsx (2)
4-9: Improve documentation clarity.The current documentation could be more explicit about the purpose and behavior differences between platforms.
/** - * Android will always make a sub menu item, so instead this will just return the item - * iOS seems to work fine with the item, so this will just return the item + * Handles platform-specific menu item display behavior: + * - On iOS: Wraps the item in a parent menu item with displayInline=true + * - On Android: Returns the original item as Android handles sub-menu items differently * @param item RN Menu Action * @returns RN Menu Action */
10-19: Consider adding type safety for subactions.The function looks good, but we could enhance type safety for the iOS case.
-export const getInlinedItem = (item: RNMenuAction): RNMenuAction => { +type InlinedMenuAction = RNMenuAction & { subactions?: RNMenuAction[] }; +export const getInlinedItem = (item: RNMenuAction): InlinedMenuAction => { if (Platform.OS === "ios") { return { title: "", displayInline: true, subactions: [item], }; } return item; };design-system/Menu/Menu.tsx (3)
27-41: Optimize themedActions memoization.The
useMemodependency array could be optimized to prevent unnecessary recalculations.- }, [actions, theme.colors.global.primary]); + }, [ + actions, + theme.colors.global.primary, + Platform.OS // Add Platform.OS since it's used in the calculation + ]);Also, consider extracting the theme colors to reduce the dependency array size:
const primaryColor = theme.colors.global.primary; const themedActions = useMemo(() => { // ... same logic }, [actions, primaryColor, Platform.OS]);
31-37: Document Android-specific color limitation.The comment about Android's background color limitation should be moved to JSDoc for better visibility.
+/** + * @platform android + * Known Issue: On Android 13, menu items only show white background. + * This may be a platform limitation or library bug. + * TODO: Investigate fix in @react-native-menu/menu library + */ -// AR: Right now Android will only show a background color of white, so don't set a titleColor which is android specific anyways -// This may be only an android 13 issue though, will either need to fix the library or wait for a fix
13-15: Enhance type definition with color validation.Consider adding color type validation for better type safety.
+type ValidColor = string & { __brand: 'ValidColor' }; + export type IMenuAction = Omit<RNMenuAction, "titleColor" | "imageColor"> & { - color?: string; + color?: ValidColor | string; // ValidColor for design system colors, string for custom colors };features/conversation/conversation-composer/conversation-composer-add-attachment-button.tsx (2)
Line range hint
136-149: Extract magic numbers to theme constants.The component uses hardcoded values from Figma. Consider moving these to the theme system.
+// In theme/metrics.ts +export const iconButtonSize = { + medium: 36, + iconSize: 20, +} as const; - style={{ - height: 36, // Value from Figma - width: 36, // Value from Figma + style={{ + height: theme.iconButtonSize.medium, + width: theme.iconButtonSize.medium, backgroundColor: theme.colors.fill.minimal, - borderRadius: 36, + borderRadius: theme.iconButtonSize.medium, }} > <Icon color={theme.colors.text.secondary} icon="plus" - size={20} // Value from figma + size={theme.iconButtonSize.iconSize} />
Line range hint
100-150: Consider extracting platform-specific icons to constants.The platform-specific icon selection could be more maintainable.
+const MENU_ICONS = { + mediaLibrary: Platform.select({ + ios: "square.and.arrow.up", + android: "ic_menu_share", + }), + camera: Platform.select({ + ios: "camera", + android: "ic_menu_camera", + }), +} as const; actions={[ { id: "mediaLibrary", title: translate("photo_library"), - image: Platform.select({ - ios: "square.and.arrow.up", - android: "ic_menu_share", - }), + image: MENU_ICONS.mediaLibrary, }, { id: "camera", title: translate("camera"), - image: Platform.select({ - ios: "camera", - android: "ic_menu_camera", - }), + image: MENU_ICONS.camera, }, ]}features/conversation-list/conversation-list.screen-header.tsx (1)
102-122: Extract menu actions configuration.Consider extracting the menu actions configuration for better maintainability.
+const getMenuActions = ( + accounts: string[], + accountsProfiles: string[], + currentAccount: string +) => [ + ...accountsProfiles.map((profilePreferedName, index) => ({ + id: accounts[index], + title: shortDisplayName(profilePreferedName), + image: currentAccount === accounts[index] ? "checkmark" : "", + })), + { + displayInline: true, + id: "new-account", + title: translate("new_account"), + image: iconRegistry["new-account-card"], + }, + { + displayInline: true, + id: "app-settings", + title: translate("App settings"), + image: iconRegistry["settings"], + }, +]; - actions={[ - ...accountsProfiles.map((profilePreferedName, index) => ({ - id: accounts[index], - title: shortDisplayName(profilePreferedName), - image: currentAccount === accounts[index] ? "checkmark" : "", - })), - { - displayInline: true, - id: "new-account", - title: translate("new_account"), - image: iconRegistry["new-account-card"], - }, - { - displayInline: true, - id: "app-settings", - title: translate("App settings"), - image: iconRegistry["settings"], - }, - ]} + actions={getMenuActions(accounts, accountsProfiles, currentAccount)}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
App.tsx(1 hunks)app.config.ts(3 hunks)design-system/Menu/Menu.tsx(1 hunks)design-system/Menu/Menu.utils.tsx(1 hunks)features/conversation-list/conversation-list.screen-header.tsx(3 hunks)features/conversation/conversation-composer/conversation-composer-add-attachment-button.tsx(4 hunks)
🔇 Additional comments (4)
App.tsx (1)
56-56: Verify the necessity of ignoring KeyboardContext warning.This change appears unrelated to the PR objectives (menu selector and splash screen). Please clarify why this warning needs to be ignored. If keyboard functionality works correctly, consider:
- Ensuring
KeyboardProvideris properly set up- Investigating the root cause of the warning instead of ignoring it
✅ Verification successful
Ignoring KeyboardContext warning is correct
The warning is safely ignored because:
- On iOS, KeyboardProvider is properly implemented
- On Android, React.Fragment is intentionally used instead of KeyboardProvider for default keyboard animation
- The warning only appears on Android where KeyboardProvider isn't needed
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for KeyboardProvider usage to verify proper setup rg -l "KeyboardProvider"Length of output: 32
Script:
#!/bin/bash # Check KeyboardProvider implementation and imports rg "KeyboardProvider" App.tsx -B 2 -A 2Length of output: 1362
app.config.ts (3)
223-234: LGTM! Splash screen configuration aligns with PR objectives.The splash screen configuration properly handles dark mode by:
- Using separate images for light/dark modes
- Setting appropriate background colors
- Maintaining consistent image width
21-22: LGTM! Improved code formatting.The conditional logic formatting changes enhance readability while maintaining the same functionality.
Also applies to: 26-27, 31-32, 36-37, 75-76
235-235: Verify the necessity of adding expo-secure-store.This addition seems unrelated to the PR objectives (menu selector and splash screen). Please clarify:
- Why secure storage is needed
- If this should be in a separate PR
✅ Verification successful
The expo-secure-store addition in app.config.ts is necessary and correct.
This is a configuration fix for an already-used package. The codebase actively uses expo-secure-store for device identification and secure storage in
utils/keychain/*. The addition in app.config.ts ensures proper Expo configuration for this existing functionality.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for expo-secure-store usage rg "expo-secure-store" # Check if there are related changes in other files rg "SecureStore"Length of output: 1965
Script:
#!/bin/bash # Check if keychain-related files were modified in this PR git diff --name-only origin/main... | grep -E "keychain|secure-store" # Look for any new usages of SecureStore in modified files git diff origin/main... | grep -E "SecureStore|CONVERSE_DEVICE_ID"Length of output: 140
| <Menu | ||
| style={{ | ||
| paddingVertical: theme.spacing.sm, // TMP solution for the hitSlop not working | ||
| paddingRight: theme.spacing.sm, // TMP solution for the hitSlop not working | ||
| }} | ||
| isMenuPrimaryAction | ||
| onPressMenuItem={({ nativeEvent }) => { | ||
| Haptics.selectionAsync(); | ||
| if (nativeEvent.actionKey === "all-chats") { | ||
| onPress={(actionId: string) => { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace temporary styling solution.
The comment indicates a temporary solution for hitSlop. Consider implementing a proper solution.
- <Menu
- style={{
- paddingVertical: theme.spacing.sm, // TMP solution for the hitSlop not working
- paddingRight: theme.spacing.sm, // TMP solution for the hitSlop not working
- }}
+ <Menu
+ hitSlop={theme.spacing.sm}
+ pressableProps={{
+ style: {
+ padding: theme.spacing.xxs,
+ },
+ }}Committable suggestion skipped: line range outside the PR's diff.
| if (actionId === "all-chats") { | ||
| Alert.alert("Coming soon"); | ||
| } else if (nativeEvent.actionKey === "new-account") { | ||
| } else if (actionId === "new-account") { | ||
| navigation.navigate("NewAccountNavigator"); | ||
| } else if (nativeEvent.actionKey === "app-settings") { | ||
| } else if (actionId === "app-settings") { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Implement proper feature flags for upcoming features.
Using Alert.alert("Coming soon") is not ideal for handling unimplemented features.
+const FEATURE_FLAGS = {
+ ALL_CHATS: false,
+ APP_SETTINGS: false,
+} as const;
+
+const handleUnimplementedFeature = (featureName: string) => {
+ Alert.alert(
+ translate("feature_coming_soon"),
+ translate("feature_coming_soon_description", { feature: featureName })
+ );
+};
onPress={(actionId: string) => {
if (actionId === "all-chats") {
- Alert.alert("Coming soon");
+ handleUnimplementedFeature("All Chats");
} else if (actionId === "new-account") {
navigation.navigate("NewAccountNavigator");
} else if (actionId === "app-settings") {
- Alert.alert("Coming soon");
+ handleUnimplementedFeature("App Settings");
}Committable suggestion skipped: line range outside the PR's diff.
| onPress?: (actionId: string) => void; | ||
| }; | ||
|
|
||
| export const Menu = ({ children, actions, style, onPress }: MenuProps) => { |
There was a problem hiding this comment.
Could we name this DropdownMenu instead please and the file dropdown-menu.tsx
There was a problem hiding this comment.
Oh sorry missed this, I will make an update for that
Added design system menu
Fixed splash screen
for dark mode
Summary by CodeRabbit
Release Notes
New Features
Menucomponent to the design systemImprovements
Chores