feat(menu): add subtitle, destructive and theme support for menu items#8
Merged
sbaiahmed1 merged 3 commits intomainfrom Dec 6, 2025
Merged
feat(menu): add subtitle, destructive and theme support for menu items#8sbaiahmed1 merged 3 commits intomainfrom
sbaiahmed1 merged 3 commits intomainfrom
Conversation
- Add subtitle and destructive properties to menu items for both iOS and Android - Implement themeVariant support for dark/light/system themes - Add title support for menu dialogs - Update example app to showcase new features
Reviewer's GuideAdds subtitle, destructive, themeVariant and title support to menu items on both iOS and Android and updates the example app to demonstrate these features. Sequence diagram for updated menu rendering and selection flow with themeVariant and destructive itemssequenceDiagram
actor User
participant App as ReactApp
participant JSMenu as MenuViewNativeComponent
participant AndroidView as AndroidMenuView
participant IOSView as IOSMenuView
User->>App: Open screen with complex menu
App->>JSMenu: Render MenuView(title, themeVariant="dark", menuItems[subtitle, destructive])
JSMenu-->>AndroidView: setTitle(title)
JSMenu-->>AndroidView: setThemeVariant("dark")
JSMenu-->>AndroidView: setMenuItems(menuItems)
JSMenu-->>IOSView: updateProps(title, themeVariant, menuItems)
AndroidView->>AndroidView: getBackgroundColor()/getTextColor() based on themeVariant
AndroidView->>AndroidView: Build dialog with header, subtitles, destructive styling
IOSView->>IOSView: Apply overrideUserInterfaceStyle from themeVariant
IOSView->>IOSView: Map menuItems to UIActions
IOSView->>IOSView: Set action.subtitle and destructive attributes
User-->>AndroidView: Tap destructive item (Android)
AndroidView->>AndroidView: selectMenuItem(identifier, title)
AndroidView-->>JSMenu: onMenuSelect(nativeEvent)
JSMenu-->>App: handleMenuSelect(event)
User-->>IOSView: Tap destructive item (iOS)
IOSView->>IOSView: selectMenuItem(identifier, title)
IOSView-->>JSMenu: onMenuSelect(nativeEvent)
JSMenu-->>App: handleMenuSelect(event)
Class diagram for updated MenuView native components and propsclassDiagram
class MenuItem {
+string identifier
+string title
+string subtitle
+bool destructive
+string iosSymbol
}
class NativeProps {
+string title
+string themeVariant
+string color
+string checkedColor
+string uncheckedColor
+MenuItem[] menuItems
}
class MenuViewNativeComponent {
+render(props NativeProps) void
}
class AndroidMenuView {
-String themeVariant
-String title
-List~Map~ menuItems
-String selectedItemIdentifier
-String checkedColor
-String uncheckedColor
+setTitle(title String) void
+setThemeVariant(themeVariant String) void
+setMenuItems(menuItems ReadableArray) void
-getBackgroundColor() int
-getTextColor() int
-selectMenuItem(identifier String, title String) void
}
class AndroidMenuViewManager {
+createViewInstance(reactContext ReactContext) AndroidMenuView
+setTitle(view AndroidMenuView, title String) void
+setThemeVariant(view AndroidMenuView, themeVariant String) void
+setColor(view AndroidMenuView, color String) void
}
class IOSMenuView {
-NSArray menuItems
-NSString selectedIdentifier
+updateProps(props MenuViewProps, oldProps MenuViewProps) void
+updateMenuItems(menuItems NSArray, selectedIdentifier NSString) void
}
class MenuViewProps {
+string title
+string themeVariant
+MenuItem[] menuItems
}
MenuViewNativeComponent --> NativeProps : uses
NativeProps "*" --> MenuItem : contains
AndroidMenuViewManager --> AndroidMenuView : creates
AndroidMenuViewManager --> AndroidMenuView : setsTitle/themeVariant
IOSMenuView --> MenuViewProps : reads
IOSMenuView --> MenuItem : buildsFrom
MenuViewProps --> MenuItem : contains
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- In
MenuViewNativeComponent.ts, consider narrowingthemeVariant?: stringto a string union type (e.g.'light' | 'dark' | 'system') so that JS/TS callers get compile-time validation and autocompletion instead of passing arbitrary strings. - On Android,
getBackgroundColor()andgetTextColor()duplicate the sameUI_MODE_NIGHT_*branching logic; you could factor this into a small helper to avoid divergence if the theme handling needs to change later.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `MenuViewNativeComponent.ts`, consider narrowing `themeVariant?: string` to a string union type (e.g. `'light' | 'dark' | 'system'`) so that JS/TS callers get compile-time validation and autocompletion instead of passing arbitrary strings.
- On Android, `getBackgroundColor()` and `getTextColor()` duplicate the same `UI_MODE_NIGHT_*` branching logic; you could factor this into a small helper to avoid divergence if the theme handling needs to change later.
## Individual Comments
### Comment 1
<location> `ios/MenuView.mm:341-348` </location>
<code_context>
+ // For now, since title was removed from props based on user request, we use empty string
+ // But if we wanted to support title later:
+ NSString *menuTitle = @"";
+ if (_props) {
+ const auto &viewProps = *std::static_pointer_cast<MenuViewProps const>(_props);
+ if (!viewProps.title.empty()) {
+ menuTitle = [[NSString alloc] initWithUTF8String:viewProps.title.c_str()];
+ }
+ }
+
+ menu = [UIMenu menuWithTitle:menuTitle children:actions];
_menuButton.menu = menu;
}
</code_context>
<issue_to_address>
**issue (bug_risk):** Menu title changes won’t trigger a menu rebuild, so updated titles may not appear.
The menu title is taken from `viewProps.title` only when the menu is created, but `menuItemsChanged` only reacts to item-level changes. If only `title` changes, `_menuButton.menu` is not rebuilt and the new title is never shown. Please either incorporate `viewProps.title` into the change detection or explicitly trigger a rebuild when the title changes.
</issue_to_address>
### Comment 2
<location> `android/src/main/java/com/menu/MenuView.kt:256` </location>
<code_context>
+ LinearLayout.LayoutParams.MATCH_PARENT,
+ (0.5 * context.resources.displayMetrics.density).toInt()
+ )
+ setBackgroundColor(if (themeVariant == "dark") Color.parseColor("#38383A") else Color.parseColor("#E0E0E0"))
+ }
+ container.addView(separator)
</code_context>
<issue_to_address>
**suggestion:** Divider color ignores system dark mode when `themeVariant` is `system`, causing a potential visual mismatch.
Here the separator color is based only on `themeVariant == "dark"`. When `themeVariant` is `system` and the device is in dark mode, `getBackgroundColor()` becomes dark but separators still use the light color, causing a mismatch. Consider deriving the divider color from `getBackgroundColor()`/`getTextColor()`, or reusing the same `uiMode` logic so system dark mode stays visually consistent.
</issue_to_address>
### Comment 3
<location> `android/src/main/java/com/menu/MenuView.kt:317-326` </location>
<code_context>
+ // Let's try the container approach where the RadioButton is the "checkmark"
+ // and the whole row is clickable.
+
+ val itemContainer = LinearLayout(context).apply {
+ orientation = LinearLayout.HORIZONTAL
+ layoutParams = LinearLayout.LayoutParams(
</code_context>
<issue_to_address>
**suggestion (bug_risk):** Wrapping the RadioButton in a clickable container may affect accessibility and RadioGroup semantics.
Using the `LinearLayout` as the clickable target and disabling clicks on the `RadioButton` can break expected `RadioGroup` behavior and accessibility (focus handling, TalkBack announcements, etc.). Consider either forwarding `itemContainer` clicks to `radioButton.performClick()` so accessibility services still see a standard radio interaction, and/or giving `itemContainer` an appropriate accessibility role/label so the whole row is exposed as a single selectable option.
</issue_to_address>
### Comment 4
<location> `src/MenuViewNativeComponent.ts:21` </location>
<code_context>
export interface NativeProps extends ViewProps {
+ title?: string;
+ themeVariant?: string;
color?: string;
checkedColor?: string;
</code_context>
<issue_to_address>
**suggestion:** Consider tightening the `themeVariant` type to the supported values instead of plain `string`.
On native platforms this is treated as `'light' | 'dark' | 'system'` with a default of `system`. Keeping it as `string` allows unsupported values that fall back unexpectedly. Consider defining `type ThemeVariant = 'light' | 'dark' | 'system';` and using `themeVariant?: ThemeVariant;` to enforce valid values at compile time.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by Sourcery
Add support for titled, themable menus with richer item metadata across iOS and Android, and update the example app to showcase these capabilities.
New Features:
Enhancements: