-
Notifications
You must be signed in to change notification settings - Fork 7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor: Modernize overlay architecture and improve bottom sheet UX #1479
Conversation
WalkthroughThis pull request introduces a redesign of UI components, transitioning from portal-based displays to modal-based implementations. Key changes include the replacement of Changes
Possibly related PRs
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 (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
features/conversation/conversation-message/conversation-message-context-menu/conversation-message-context-menu.tsx (1)
129-197
: Consider adding accessibility supportThe Modal implementation could benefit from accessibility enhancements:
- Add
accessibilityViewIsModal
prop to indicate modal behavior to screen readers- Add
accessibilityLiveRegion
for dynamic content updates<Modal visible={true} transparent={true} onRequestClose={handlePressBackdrop} animationType="fade" statusBarTranslucent={Platform.OS === "android"} supportedOrientations={["portrait", "landscape"]} + accessibilityViewIsModal={true} + accessibilityLiveRegion="polite" >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
features/conversation/conversation-message/conversation-message-context-menu/conversation-message-context-menu-backdrop.tsx
(2 hunks)features/conversation/conversation-message/conversation-message-context-menu/conversation-message-context-menu.tsx
(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: tsc
- GitHub Check: performance-test
- GitHub Check: test
- GitHub Check: lint
🔇 Additional comments (3)
features/conversation/conversation-message/conversation-message-context-menu/conversation-message-context-menu-backdrop.tsx (1)
4-4
: LGTM! Clean theme integrationThe theme integration is well-implemented, making the backdrop's blur effect responsive to the app's theme. The conditional tint provides appropriate visual feedback in both light and dark modes.
Also applies to: 14-14, 17-17
features/conversation/conversation-message/conversation-message-context-menu/conversation-message-context-menu.tsx (2)
129-136
: LGTM! Well-structured Modal migrationThe Portal to Modal migration is well-implemented with proper configuration:
- Transparent background
- Backdrop press handling
- Platform-specific status bar handling
- Orientation support
137-140
: Verify animation performance on low-end devicesThe animation durations look good, but we should verify the performance on low-end devices.
✅ Verification successful
Animation implementation looks optimal for performance
The 200ms fade animation is appropriate for a context menu overlay:
- Uses performant react-native-reanimated library
- Duration is shorter than other animations in the codebase (500ms)
- Simple fade effect on a small UI element
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other FadeIn/FadeOut animations in the codebase to compare durations rg --type typescript "FadeIn.duration|FadeOut.duration" -B 2 -A 2Length of output: 102
Script:
#!/bin/bash # Search for FadeIn/FadeOut animations without file type restriction rg "FadeIn\.duration|FadeOut\.duration" -B 2 -A 2 # Search for animation-related imports to understand the context rg "import.*Animated" -B 0 -A 1 # Search for other duration-based animations rg "\.duration\((\d+)\)" -B 2 -A 2Length of output: 15175
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 |
- Remove @gorhom/portal dependency - Replace Portal usage with React Native Modal - Replace BottomSheetModal with standard BottomSheet - Fix bottom sheet behavior & responsiveness - Fix snap points by disabling dynamic sizing - Improve dismiss/reopen flow
c6465b4
to
c5659c7
Compare
- Set default filter to "All" when opening reactions drawer - Fix empty reactions list on subsequent drawer opens
...ion-message/conversation-message-context-menu/conversation-message-context-menu-backdrop.tsx
Outdated
Show resolved
Hide resolved
onChange={(index) => { | ||
if (index === -1) { | ||
handleDismiss(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doens'T make sense. Same as other comment with onAnimate
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do this to make sure to dismiss the wrapping modal when the bottom sheet is dismissed
...sage-reactions/conversation-message-reaction-drawer/conversation-message-reaction-drawer.tsx
Outdated
Show resolved
Hide resolved
...sage-reactions/conversation-message-reaction-drawer/conversation-message-reaction-drawer.tsx
Outdated
Show resolved
Hide resolved
...sage-reactions/conversation-message-reaction-drawer/conversation-message-reaction-drawer.tsx
Outdated
Show resolved
Hide resolved
...sage-reactions/conversation-message-reaction-drawer/conversation-message-reaction-drawer.tsx
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 4
♻️ Duplicate comments (4)
features/conversation/conversation-message/conversation-message-reactions/conversation-message-reaction-drawer/conversation-message-reaction-drawer.service.ts (1)
11-11
: 🛠️ Refactor suggestionRemove unnecessary type assertions
The type assertions to
IMessageReactionsStore
are unnecessary as Zustand'screate
function already provides proper typing for the store state.- const store = useMessageReactionsStore.getState() as IMessageReactionsStore; + const store = useMessageReactionsStore.getState();Also applies to: 18-18
features/conversation/conversation-message/conversation-message-reactions/conversation-message-reaction-drawer/conversation-message-reaction-drawer.tsx (3)
13-13
: 🛠️ Refactor suggestionUse the custom 'TouchableHighlight' component.
As previously suggested, please import and use our custom
TouchableHighlight
component instead of the one fromreact-native
.Apply this diff to import the custom component:
-import { - TextStyle, - TouchableHighlight, - ViewStyle, - Modal, - Platform, -} from "react-native"; +import { TextStyle, ViewStyle, Modal, Platform } from "react-native"; +import TouchableHighlight from "@components/TouchableHighlight";
74-77
: 🛠️ Refactor suggestionReview the logic in 'onChange' callback regarding 'index === -1'.
As previously noted, the condition
index === -1
may not make sense in this context. Please verify whether this check is necessary and adjust the logic accordingly.🧰 Tools
🪛 GitHub Check: tsc
[failure] 74-74:
Parameter 'index' implicitly has an 'any' type.
79-84
: 🛠️ Refactor suggestionRe-evaluate the logic in 'onAnimate' callback regarding 'toIndex === -1'.
As previously noted, the condition
toIndex === -1
may be unnecessary because iftoIndex
is -1, the bottom sheet is already closing. Consider simplifying the logic accordingly.🧰 Tools
🪛 GitHub Check: tsc
[failure] 79-79:
Parameter 'fromIndex' implicitly has an 'any' type.
[failure] 79-79:
Parameter 'toIndex' implicitly has an 'any' type.
🧹 Nitpick comments (6)
features/conversation/conversation-message/conversation-message-reactions/conversation-message-reaction-drawer/conversation-message-reaction-drawer.store.ts (2)
4-9
: Consider removing the "I" prefix from interface nameThe "I" prefix for interfaces is considered outdated in modern TypeScript. Consider renaming
IMessageReactionsStore
toMessageReactionsStore
to align with current TypeScript conventions.-export type IMessageReactionsStore = { +export type MessageReactionsStore = {
25-35
: Consider extracting initial state to avoid duplicationThe initial state is duplicated between the store creation and reset function. Consider extracting it to a constant to maintain DRY principles and ease future maintenance.
+const initialState = { + rolledUpReactions: { + preview: [], + detailed: [], + totalCount: 0, + userReacted: false, + }, + isVisible: false, +}; export const useMessageReactionsStore = create<IMessageReactionsStore>( - (set) => ({ - rolledUpReactions: { - preview: [], - detailed: [], - totalCount: 0, - userReacted: false, - }, - isVisible: false, + (set) => ({ + ...initialState, setRolledUpReactions: (reactions) => set({ rolledUpReactions: reactions }), setIsVisible: (isVisible) => set({ isVisible }), }) ); export function resetMessageReactionsStore() { - useMessageReactionsStore.setState({ - rolledUpReactions: { - preview: [], - detailed: [], - totalCount: 0, - userReacted: false, - }, - isVisible: false, - }); + useMessageReactionsStore.setState(initialState); }features/conversation/conversation-message/conversation-message-reactions/conversation-message-reaction-drawer/conversation-message-reaction-drawer.service.ts (1)
Line range hint
25-27
: Consider removing redundant reset functionThe
resetMessageReactionsDrawer
function appears redundant as it simply forwards the call toresetMessageReactionsStore
. Consider removing it and usingresetMessageReactionsStore
directly at call sites.design-system/BlurView.tsx (1)
4-4
: LGTM! Consider adding prop type documentation.The theme-based tint implementation looks good and aligns well with the modernization efforts. The dynamic tint selection based on the theme enhances the component's adaptability.
Consider adding JSDoc documentation for the
tint
prop to help other developers understand its purpose and accepted values:export type IBlurViewProps = IVStackProps & - AnimatedProps<BlurViewProps> & { isAbsolute?: boolean }; + AnimatedProps<BlurViewProps> & { + /** Whether the BlurView should be positioned absolutely */ + isAbsolute?: boolean; + /** Override the default theme-based tint. Accepts 'light' | 'dark' */ + tint?: BlurViewProps['tint']; + };Also applies to: 17-17, 20-21, 25-25
package.json (1)
49-49
: Consider documenting the reason for version pinning.The change from
^5.0.2
to5.0.6
for@gorhom/bottom-sheet
pins the version. While this can help with stability, it's good practice to document the reasoning.Consider adding a comment in the PR description or commit message explaining why this specific version was chosen.
features/conversation/conversation-message/conversation-message-reactions/conversation-message-reaction-drawer/conversation-message-reaction-drawer.tsx (1)
71-71
: Remove unnecessary prop 'animateOnMount'.The prop
animateOnMount
defaults totrue
, so it can be omitted unless you need to set it tofalse
.Apply this diff to remove the unnecessary prop:
- animateOnMount
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
yarn.lock
is excluded by!**/yarn.lock
,!**/*.lock
📒 Files selected for processing (9)
App.tsx
(1 hunks)components/Drawer.tsx
(2 hunks)design-system/BlurView.tsx
(2 hunks)design-system/BottomSheet/BottomSheet.tsx
(1 hunks)features/conversation/conversation-message/conversation-message-context-menu/conversation-message-context-menu.tsx
(4 hunks)features/conversation/conversation-message/conversation-message-reactions/conversation-message-reaction-drawer/conversation-message-reaction-drawer.service.ts
(2 hunks)features/conversation/conversation-message/conversation-message-reactions/conversation-message-reaction-drawer/conversation-message-reaction-drawer.store.ts
(1 hunks)features/conversation/conversation-message/conversation-message-reactions/conversation-message-reaction-drawer/conversation-message-reaction-drawer.tsx
(1 hunks)package.json
(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- features/conversation/conversation-message/conversation-message-context-menu/conversation-message-context-menu.tsx
🧰 Additional context used
🪛 GitHub Check: tsc
features/conversation/conversation-message/conversation-message-reactions/conversation-message-reaction-drawer/conversation-message-reaction-drawer.tsx
[failure] 4-4:
Module '"@design-system/BottomSheet/BottomSheet"' has no exported member 'CustomBottomSheet'.
[failure] 74-74:
Parameter 'index' implicitly has an 'any' type.
[failure] 79-79:
Parameter 'fromIndex' implicitly has an 'any' type.
[failure] 79-79:
Parameter 'toIndex' implicitly has an 'any' type.
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: performance-test
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (7)
features/conversation/conversation-message/conversation-message-reactions/conversation-message-reaction-drawer/conversation-message-reaction-drawer.store.ts (1)
11-23
: Well-structured store implementation!The store implementation follows Zustand best practices with proper typing and immutable state updates.
features/conversation/conversation-message/conversation-message-reactions/conversation-message-reaction-drawer/conversation-message-reaction-drawer.service.ts (2)
8-22
: Clean implementation of drawer visibility managementThe implementation effectively manages drawer state using the new store, aligning well with the PR's objective of modernizing the overlay architecture.
29-35
: Well-implemented selector hooks!The selector hooks are well-designed, providing focused access to specific parts of the store state.
components/Drawer.tsx (1)
170-196
: Modal implementation enhances Drawer presentationThe refactoring to use React Native's
Modal
component effectively replaces thePortal
, providing better control over the Drawer presentation and aligning with best practices.design-system/BottomSheet/BottomSheet.tsx (1)
1-72
: 'BottomSheet' component is well-implementedThe new
BottomSheet
component correctly encapsulatesBottomSheetBase
and integrates theming by combining styles withuseMemo
. The use offorwardRef
allows proper ref forwarding to the underlying component.package.json (1)
49-49
: Verify bottom-sheet version update impact.The update to bottom-sheet v5.0.6 might include breaking changes or require additional updates.
Let's check for any potential issues:
✅ Verification successful
Bottom sheet version update appears safe due to proper abstraction
The codebase implements a robust design system wrapper around @gorhom/bottom-sheet, isolating direct usage and providing type-safe abstractions. All bottom-sheet components are properly encapsulated within the design system, making the version update less likely to cause issues.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for bottom-sheet usage patterns that might need updates echo "Checking bottom-sheet component usage..." ast-grep --pattern 'import { $_ } from "@gorhom/bottom-sheet"' # Search for potential breaking patterns echo "Checking for potential breaking patterns..." rg "BottomSheetModal" -A 5Length of output: 22570
features/conversation/conversation-message/conversation-message-reactions/conversation-message-reaction-drawer/conversation-message-reaction-drawer.tsx (1)
62-62
: Use 'AnimatedVStack' instead of 'Animated.View'.As previously suggested, please use our design system components like
AnimatedVStack
for consistency.Apply this diff to use
AnimatedVStack
:-import Animated, { FadeIn, FadeOut } from "react-native-reanimated"; +import { AnimatedVStack } from "@design-system/VStack"; ... - <Animated.View style={{ flex: 1 }}> + <AnimatedVStack style={{ flex: 1 }}>
...sage-reactions/conversation-message-reaction-drawer/conversation-message-reaction-drawer.tsx
Outdated
Show resolved
Hide resolved
...sage-reactions/conversation-message-reaction-drawer/conversation-message-reaction-drawer.tsx
Outdated
Show resolved
Hide resolved
...sage-reactions/conversation-message-reaction-drawer/conversation-message-reaction-drawer.tsx
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Small loom for more feedback but approved because it might not be relevant and priority https://www.loom.com/share/d941330aa95c4ccb816cb845d0246a3f
...conversation-message/conversation-message-context-menu/conversation-message-context-menu.tsx
Show resolved
Hide resolved
Thanks! I've simplified the reactions drawer implementation:
Code is now more straightforward with fewer moving parts and clearer state management. The drawer shows when reactions are present and dismisses cleanly when the bottom sheet is closed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (4)
features/conversation/conversation-message/conversation-message-reactions/conversation-message-reaction-drawer/conversation-message-reaction-drawer.tsx (4)
15-15
: Consider using a more specific type import.Instead of importing the entire types file, consider importing just the
BottomSheetMethods
interface for better tree-shaking:-import { BottomSheetMethods } from "@gorhom/bottom-sheet/lib/typescript/types"; +import type { BottomSheetMethods } from "@gorhom/bottom-sheet/lib/typescript/types";
56-60
: Add type safety to the onChange callback.Specify the type for the index parameter:
-onChange={(index) => { +onChange={(index: number) => {
49-62
: Consider optimizing BottomSheet props.Some props might be redundant as they are already defaults:
<BottomSheet ref={bottomSheetRef} snapPoints={["65%", "90%"]} index={0} enablePanDownToClose enableOverDrag={false} - enableDynamicSizing={false} onChange={(index: number) => { if (index === -1) { handleDismiss(); } }} topInset={insets.top} >
67-74
: Consider optimizing ScrollView performance.Add
removeClippedSubviews
prop to optimize memory usage when many items are present:<ScrollView horizontal showsHorizontalScrollIndicator={false} + removeClippedSubviews={true} style={{ paddingLeft: theme.spacing.lg, flexDirection: "row", }} scrollEnabled >
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
components/Drawer.tsx
(2 hunks)features/conversation/conversation-message/conversation-message-context-menu/conversation-message-context-menu.tsx
(4 hunks)features/conversation/conversation-message/conversation-message-reactions/conversation-message-reaction-drawer/conversation-message-reaction-drawer.service.ts
(1 hunks)features/conversation/conversation-message/conversation-message-reactions/conversation-message-reaction-drawer/conversation-message-reaction-drawer.store.ts
(1 hunks)features/conversation/conversation-message/conversation-message-reactions/conversation-message-reaction-drawer/conversation-message-reaction-drawer.tsx
(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- features/conversation/conversation-message/conversation-message-context-menu/conversation-message-context-menu.tsx
- features/conversation/conversation-message/conversation-message-reactions/conversation-message-reaction-drawer/conversation-message-reaction-drawer.store.ts
- components/Drawer.tsx
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: performance-test
- GitHub Check: build
- GitHub Check: test
🔇 Additional comments (3)
features/conversation/conversation-message/conversation-message-reactions/conversation-message-reaction-drawer/conversation-message-reaction-drawer.service.ts (1)
8-13
: LGTM! Clean implementation using store management.The refactored implementation properly separates concerns by focusing on state management through Zustand store instead of handling modal refs directly.
features/conversation/conversation-message/conversation-message-reactions/conversation-message-reaction-drawer/conversation-message-reaction-drawer.tsx (2)
29-37
: LGTM! Well-structured dismiss handler with clear responsibilities.The centralized dismiss handler properly manages all cleanup tasks, preventing potential state mismatches.
131-153
: LGTM! Efficient implementation of the detailed reactions list.Good use of FlashList for virtualized rendering and proper key generation for optimal performance.
This PR modernizes our overlay architecture by partially removing the Portal dependency and improving the bottom sheet implementation. It also fixes the reaction context menu that has not worked since the Expo 52 update because of the use of Portal and incompatibility with our ThemeProvider.
Key Changes:
@gorhom/portal
dependency in favor of React Native's Modal (still needed by some other parts of the app)BottomSheetModal
with standardBottomSheet
wrapped inModal
Summary by CodeRabbit
Release Notes
New Features
BottomSheet
component for customizable stylesImprovements
Technical Updates
Dependency Changes
@gorhom/bottom-sheet
package@gorhom/portal
package