fix: Fixed Buttons for Certain cases #1127
Conversation
Removed deprecated uses Updated props Fixed navigation to pop to top
WalkthroughThe changes introduced in this pull request primarily focus on updating several components to utilize a centralized design system for UI elements, specifically the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
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
🧹 Outside diff range and nitpick comments (9)
design-system/Button/Button.props.ts (1)
Line range hint
13-21: Consider cleaning up deprecated button variants.The
IButtonVarianttype includes a deprecatedsecondaryvariant. Since you're already working on standardizing button usage, consider planning the removal of deprecated variants to maintain a cleaner design system.Consider:
- Documenting the preferred alternatives for deprecated variants
- Creating a migration plan for removing deprecated variants
- Updating the design system documentation to reflect these changes
components/Chat/ChatPlaceholder/GroupChatPlaceholder.tsx (1)
1-1: Good architectural improvement!The migration to use the centralized design system Button component improves maintainability and ensures UI consistency across the application.
components/Chat/ConsentPopup/ConsentPopup.tsx (1)
91-93: LGTM: Improved action hierarchyThe button changes appropriately emphasize the accept action with the 'fill' variant, creating better visual hierarchy between the actions.
The visual hierarchy now better reflects the positive action as primary, which is a good UX practice for consent flows.
components/Chat/ConsentPopup/GroupConsentPopup.tsx (1)
Line range hint
31-33: Enhance error message specificityThe error message could be more descriptive by including the missing requirements:
- throw new Error("This component should only be used for group chats"); + throw new Error( + "GroupConsentPopup requires a group chat with a valid topic and current account" + );i18n/translations/en.ts (1)
313-315: Consider adding error state translations.While the new translation keys for viewing removed chats look good, consider adding corresponding error state messages that might be needed when:
- No removed chats are available to view
- There's an error loading removed chats
Example additions:
view_removed_chats: "View removed chats", + view_removed_chats_empty: "No removed chats to display", + view_removed_chats_error: "Unable to load removed chats",screens/NewConversation/NewConversation.tsx (3)
112-117: Consider standardizing button margin handlingThe negative margin (
marginRight: -10) used to adjust button positioning could lead to inconsistent spacing across different screen sizes or when the design system spacing tokens change.Consider using the design system's spacing tokens or adjusting the parent container's padding instead:
- style={{ marginRight: -10, padding: 10 }} + style={{ padding: 10 }}
544-544: Remove commented codeThe commented height property suggests uncertainty about platform-specific styling. Either remove it or document why it might be needed in the future.
- // height: Platform.OS === "ios" ? 30 : undefined,
Based on the script results, I can generate the final response as we have sufficient information to verify the button migration status.
Migration to design system Button is incomplete
There are still instances of the old
ConverseButtoncomponent inscreens/TopUp.tsxthat need to be migrated to the new design systemButton. Additionally, there are some inconsistencies in button props:
- Some buttons use
titleprop while others usetextprop- Found in
NewConversationModal.tsxandNewGroupSummary.tsx🔗 Analysis chain
Line range hint
1-565: Verify complete migration to design system ButtonLet's ensure all button instances have been migrated to the new design system component.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for any remaining instances of ConverseButton rg "ConverseButton" # Verify consistent usage of Button props rg -A 2 "Button.*variant"Length of output: 6066
screens/Profile.tsx (1)
Line range hint
733-762: Consider enhancing notification permission error handlingThe notification permission request flow could benefit from more robust error handling and user feedback. Consider extracting this logic into a dedicated hook or utility function that provides:
- Centralized error handling
- Clear user feedback for failed permission requests
- Consistent behavior across platforms
Example implementation:
// hooks/useNotificationPermissions.ts export const useNotificationPermissions = () => { const requestPermissions = async (): Promise<{status: NotificationPermissionStatus; error?: Error}> => { try { if (Platform.OS === 'android') { const status = await requestPushNotificationsPermissions(); if (status === 'denied') { return { status, error: new Error('Permissions denied') }; } return { status }; } // iOS implementation const status = await requestPushNotificationsPermissions(); return { status }; } catch (error) { return { status: 'denied', error: error as Error }; } }; return { requestPermissions }; };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
- components/Chat/ChatPlaceholder/ChatPlaceholder.tsx (3 hunks)
- components/Chat/ChatPlaceholder/GroupChatPlaceholder.tsx (2 hunks)
- components/Chat/ConsentPopup/ConsentPopup.tsx (3 hunks)
- components/Chat/ConsentPopup/GroupConsentPopup.tsx (2 hunks)
- design-system/Button/Button.props.ts (1 hunks)
- i18n/translations/en.ts (1 hunks)
- screens/NewConversation/NewConversation.tsx (6 hunks)
- screens/Profile.tsx (3 hunks)
🔇 Additional comments (14)
design-system/Button/Button.props.ts (1)
Line range hint
1-109: Well-structured Button interface with comprehensive prop definitions.The
IButtonPropsinterface is well-designed with:
- Clear TypeScript types and documentation
- Proper extension of React Native's
PressableProps- Comprehensive set of styling options
- Good support for accessibility through proper prop types
components/Chat/ChatPlaceholder/GroupChatPlaceholder.tsx (1)
96-98: Verify design system Button props usageThe Button props look appropriate for a primary action, but let's ensure consistent usage across similar contexts.
✅ Verification successful
Let me gather more information to verify the Button usage consistency specifically in chat-related components.
Let me check one more specific pattern to see how primary action buttons are implemented in chat components.
Button props usage is consistent with design system patterns
The verification shows that the Button props are correctly implemented and consistent across chat placeholders and similar contexts:
- Both ChatPlaceholder and GroupChatPlaceholder use
variant="fill"for primary actions- The "Say hi" action consistently uses
picto="hand.wave"in placeholder components- Primary actions in consent popups and other important interactions also use
variant="fill"🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other Button usages in chat placeholders to verify consistency ast-grep --pattern 'Button $$$variant="fill"$$$' # Check for any remaining old button variants that might need updating rg -A 2 'variant="outline"'Length of output: 757
Script:
#!/bin/bash # Search for Button components in chat-related files rg -l "Button" components/Chat/ # Check specific Button usage patterns in chat components ast-grep --pattern 'Button $$$ variant=$_ $$$ ' # Look for any say_hi related components rg -A 3 "say_hi"Length of output: 1631
Script:
#!/bin/bash # Look for Button components with variant prop in Chat components rg -A 2 'variant=' components/Chat/ # Check for other primary action buttons in placeholders rg -A 3 'Button.*variant.*placeholder' -iLength of output: 1818
components/Chat/ConsentPopup/ConsentPopup.tsx (2)
1-1: LGTM: Design system migrationThe Button import has been correctly updated to use the centralized design system.
62-65: Verify design system button props combinationThe button props have been updated to use the new design system pattern. Please verify that this combination of
variant="text"withaction="danger"is documented and produces the intended visual state.Consider documenting these button variant combinations in the design system if not already done, to help other developers understand the available options.
components/Chat/ConsentPopup/GroupConsentPopup.tsx (2)
2-2: LGTM: Design system migrationThe Button import has been correctly updated to use the centralized design system.
95-96: Verify the button hierarchy for consent actionsThe prop updates from
titletotextalign with the design system. However, changing the "Join" button variant from "secondary" to "fill" might affect the visual hierarchy of these critical consent actions.Let's verify if this variant change is consistent with other consent-related UIs:
Also applies to: 100-102
✅ Verification successful
Let's try a broader search to understand the consent-related UI patterns and button hierarchy in the codebase. The previous command had issues with file type recognition, so I've adjusted the search parameters.
Button hierarchy is consistent with consent UI patterns
The button variant changes are consistent with the consent UI patterns in the codebase. Looking at both
GroupConsentPopup.tsxandConsentPopup.tsx, they follow the same pattern:
- Decline/Block actions use
variant="text"- Accept/Join actions use
variant="fill"with a checkmark pictoThis hierarchy appropriately emphasizes the positive action (join/accept) while making the negative action (decline/block) less prominent but still accessible.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other consent-related Button components to verify variant consistency rg -U --type tsx --type jsx 'Button.*variant.*consent' -B 2 -A 2 # Search for similar action pairs (decline/accept) to verify button hierarchy ast-grep --pattern 'Button.*variant=(\"text\"|\"fill\")'Length of output: 153
Script:
#!/bin/bash # Search for other consent-related UI components rg -t ts -t tsx -t jsx "consent" -B 3 -A 3 # Search for Button components with variant props in TypeScript/React files rg -t ts -t tsx -t jsx "Button.*variant" -B 2 -A 2 # Search specifically in the ConsentPopup directory fd . -e tsx -e ts -e jsx "components/Chat/ConsentPopup" --exec cat {}Length of output: 8932
components/Chat/ChatPlaceholder/ChatPlaceholder.tsx (3)
1-1: LGTM: Design system import aligns with standardization effortThe updated import path properly utilizes the centralized design system, which promotes consistency across the application.
127-129: Verify emoji consistency across greeting actionsThe "Say hi" button sends a wave emoji (👋). Let's ensure this is consistent with other greeting actions across the app.
#!/bin/bash # Search for other greeting-related code to verify emoji consistency rg -A 3 'Say hi|sendMessage.*wave|👋'
81-83: Verify visual hierarchy for unblock actionWhile the updated button props align with the design system, please verify that the "fill" variant provides sufficient visual emphasis for this important user action. Consider if this critical function should use a different variant to stand out in the UI.
✅ Verification successful
The previous search didn't yield relevant results, so I'm broadening the search to:
- Understand how "fill" variant buttons are used in the design system
- Find all block/unblock related UI patterns for consistency check
Let's try these modified searches to:
- Find Button usage patterns without file type restriction
- Look for block/unblock patterns specifically in components
- Examine the immediate context of the Button usage in ChatPlaceholder
The "fill" variant is appropriate for the unblock action
Based on the codebase analysis:
- The "fill" variant is consistently used for primary actions in the app, including the companion "Say hi" button in the same component
- The unblock action is protected by an action sheet confirmation dialog that requires explicit user confirmation
- The button appears in a clear context with explanatory text "This user is blocked" above it
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other unblock actions in the codebase to verify consistent styling rg -l 'Unblock' | xargs rg 'variant.*fill'Length of output: 2209
Script:
#!/bin/bash # Search for Button components with variant="fill" to understand design system patterns rg -A 2 'Button.*variant="fill"' # Search specifically for unblock-related UI elements rg -A 3 -i 'unblock|block' --type tsxLength of output: 464
Script:
#!/bin/bash # Search for Button components with variant="fill" in all files rg -A 2 'Button.*variant="fill"' # Search for block/unblock patterns in all files rg -A 3 -i 'unblock|block' components/ # Look for Button component usage patterns in the ChatPlaceholder component rg -A 5 'Button' components/Chat/ChatPlaceholder/ChatPlaceholder.tsxLength of output: 17533
i18n/translations/en.ts (1)
315-316: LGTM! Translation key follows conventions.The
change_or_add_accounttranslation key follows the established naming pattern and its value uses consistent capitalization.screens/NewConversation/NewConversation.tsx (2)
1-1: LGTM: Clean import of design system Button componentThe import statement correctly references the design system Button component, aligning with the PR objective to standardize button usage.
Line range hint
354-365: LGTM: Proper implementation of group member buttonsThe button implementation correctly uses the design system's variants and props for consistent styling.
screens/Profile.tsx (2)
710-712: LGTM: Navigation flow improvementThe addition of
navigation.popToTop()before navigating to the "Blocked" screen is a good improvement. This ensures a clean navigation stack and prevents potential navigation state issues.
Line range hint
722-733: LGTM: Improved internationalizationThe updates to use the
translatefunction for button titles enhance the app's internationalization capabilities. This change aligns well with maintaining a consistent localization approach throughout the application.
| /** @deprecated - use text instead */ | ||
| title?: string; | ||
| /** @deprecated */ |
There was a problem hiding this comment.
💡 Codebase verification
Migration to text prop is incomplete - action required
Several components are still using the deprecated title prop:
screens/NewConversation/NewGroupSummary.tsxscreens/NewConversation/NewConversationModal.tsx- Multiple instances in
components/__tests__/Button.test.tsx
While the new text prop is being adopted in some components:
screens/NewConversation/NewConversation.tsxdesign-system/BottomSheet/BottomSheetModalDemo.tsx
The migration is in progress but needs to be completed for consistency. Please update the remaining instances to use the text prop instead of the deprecated title prop.
🔗 Analysis chain
LGTM! Clear deprecation notice.
The deprecation notice for the title prop is clear and provides the correct alternative (text). This aligns well with the ongoing migration to standardize button properties across the application.
Let's verify the migration progress to ensure consistency:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check the usage of deprecated 'title' prop vs recommended 'text' prop in Button components
# Search for remaining uses of the deprecated 'title' prop
echo "Checking for remaining uses of deprecated 'title' prop:"
rg -A 1 '<Button[^>]*title='
# Search for uses of the recommended 'text' prop
echo -e "\nChecking for uses of recommended 'text' prop:"
rg -A 1 '<Button[^>]*text='
Length of output: 1564
| <Button | ||
| variant="text" | ||
| picto="person.2" | ||
| title="New group" | ||
| text="New group" | ||
| style={styles.newGroupButton} | ||
| onPress={() => { | ||
| setGroup({ enabled: true, members: [] }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider abstracting platform-specific styling
The button uses inline platform-specific padding which could be moved to the styles object for better maintainability.
Consider refactoring to use a dedicated style:
- style={styles.newGroupButton}
+ style={styles.newGroupButton}And in the styles object:
newGroupButton: {
marginLeft: 7,
paddingVertical: Platform.select({
ios: 13,
android: 10
})
}
Removed deprecated uses Updated props Fixed navigation to pop to top
Removed deprecated uses
Updated props
Fixed navigation to pop to top
Fixes #1126
Fixes #1123
Summary by CodeRabbit
New Features
Bug Fixes
Chores