Skip to content

Bug fix/UI feedback color changes #20

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.

Sign up for GitHub

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

Merged
Manan-Sharma-5 merged 2 commits into from
Nov 12, 2024
Merged

Bug fix/UI feedback color changes #20

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
79723f9
fixed color scheme changing in dark mode
Manan-Sharma-5 Nov 11, 2024
b5862a8
fixed imports
Manan-Sharma-5 Nov 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 15 additions & 17 deletions app/_layout.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,23 +61,21 @@ export default function RootLayout() {

return (
<GestureHandlerRootView>
<Provider>
<AuthContextProvider>
<FirebaseStorageContextProvider>
<NotificationContextProvider>
<CloudMessagingContextProvider>
<BrandContextProvider>
<CollaborationContextProvider>
<AutocompleteDropdownContextProvider>
<RootLayoutStack />
</AutocompleteDropdownContextProvider>
</CollaborationContextProvider>
</BrandContextProvider>
</CloudMessagingContextProvider>
</NotificationContextProvider>
</FirebaseStorageContextProvider>
</AuthContextProvider>
</Provider>
<AuthContextProvider>
<FirebaseStorageContextProvider>

@dev-rathore dev-rathore Nov 11, 2024

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did we removed react-native-paper Provider, isn't that required?

<NotificationContextProvider>
<CloudMessagingContextProvider>
<BrandContextProvider>
<CollaborationContextProvider>
<AutocompleteDropdownContextProvider>
<RootLayoutStack />
</AutocompleteDropdownContextProvider>
</CollaborationContextProvider>
</BrandContextProvider>
</CloudMessagingContextProvider>
</NotificationContextProvider>
</FirebaseStorageContextProvider>
</AuthContextProvider>
Comment on lines +64 to +78

@coderabbitai coderabbitai Bot Nov 11, 2024

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codebase verification

The react-native-paper Provider is required and should not be removed

The codebase extensively uses react-native-paper components, including Modals, FABs, and other components that require the Provider context. The Provider import is present in the layout file but has been removed from the component tree. This will cause styling and functionality issues for react-native-paper components throughout the application.

  • Add back the Provider from react-native-paper, wrapping it around the context providers:
<GestureHandlerRootView>
  <Provider>
    <AuthContextProvider>
      {/* other context providers */}
    </AuthContextProvider>
  </Provider>
</GestureHandlerRootView>
🔗 Analysis chain

Verify the removal of react-native-paper Provider.

While removing the unnecessary wrapper improves the component hierarchy, we should ensure that no components rely on react-native-paper's theming or components.

The search results show extensive usage of react-native-paper components throughout the codebase, including components that typically require the Provider context (like Modal, Dialog, Portal, etc.). Let's verify if there's another Provider setup elsewhere in the codebase that might be handling these components.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for react-native-paper component usage in the codebase
echo "Checking for react-native-paper component usage..."
rg -l "from 'react-native-paper'" || rg -l 'from "react-native-paper"'

# Search for specific paper components that might need the Provider
echo "Checking for specific Paper components that typically need Provider..."
rg -l "useTheme|Portal\.Host|FAB|Snackbar|Modal|Dialog"

Length of output: 3696

Script:

#!/bin/bash
# Check for any other react-native-paper Provider setup in the codebase
echo "Checking for Provider setup from react-native-paper..."
rg -A 5 "import.*Provider.*from 'react-native-paper'" || rg -A 5 'import.*Provider.*from "react-native-paper"'

# Check the specific layout file to see the complete context
echo "Checking the layout file..."
cat app/_layout.tsx

Length of output: 5318

</GestureHandlerRootView>
);
}
Expand Down
53 changes: 22 additions & 31 deletions components/InfluencerCard.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,44 +133,35 @@ const InfluencerCard = (props: InfluencerCardPropsType) => {
}}
>
<Animated.View>
<Image source={{ uri: item.url }} style={styles.media} />
<Image
source={{ uri: item.url || item.uri }}
style={styles.media}
/>
</Animated.View>
</TapGestureHandler>
);
} else {
return (
<TouchableOpacity
onPress={() => {
if (!isSwiping) {
setMuted(!muted);
if (index === currentIndex) {
setIsPlaying(!isPlaying);
}
<Video
ref={(ref) => {
if (ref) {
videoRefs.current[index] = ref;
}
}}
Comment on lines +146 to 150

@coderabbitai coderabbitai Bot Nov 11, 2024

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Prevent potential memory leaks with video refs.

The current implementation of video refs could lead to memory leaks. Consider cleaning up refs on component unmount.

Add a cleanup effect:

useEffect(() => {
  return () => {
    // Cleanup video refs on unmount
    Object.values(videoRefs.current).forEach((videoRef) => {
      if (videoRef) {
        videoRef.unloadAsync();
      }
    });
    videoRefs.current = {};
  };
}, []);

>
<Video
ref={(ref) => {
if (ref) {
videoRefs.current[index] = ref;
}
}}
source={
item.appleUrl
? {
uri: Platform.OS === "ios" ? item.appleUrl : item.playUrl,
}
: require("../assets/videos/ForBiggerJoyrides.mp4")
}
style={styles.media}
resizeMode={ResizeMode.COVER}
isLooping
isMuted={muted}
shouldPlay={isPlaying}
useNativeControls
onError={(error) => console.log("Video Error:", error)}
/>
</TouchableOpacity>
source={
item.appleUrl
? {
uri: Platform.OS === "ios" ? item.appleUrl : item.playUrl,
}
: require("../assets/videos/ForBiggerJoyrides.mp4")
}
style={styles.media}
resizeMode={ResizeMode.COVER}
isLooping={false}
shouldPlay={false}
useNativeControls
Comment on lines +158 to +162

@coderabbitai coderabbitai Bot Nov 11, 2024

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Add loading state for video playback.

The video component lacks loading state handling, which could lead to poor user experience while videos are buffering.

 <Video
   style={styles.media}
   resizeMode={ResizeMode.COVER}
   isLooping={false}
   shouldPlay={false}
   useNativeControls
+  onLoadStart={() => setLoading(index, true)}
+  onLoad={() => setLoading(index, false)}
 />
+{loading[index] && (
+  <ActivityIndicator 
+    style={styles.videoLoader}
+    size="large"
+  />
+)}

Committable suggestion skipped: line range outside the PR's diff.

onError={(error) => console.log("Video Error:", error)}

@coderabbitai coderabbitai Bot Nov 11, 2024

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

Improve video error handling.

Simply logging video errors to console isn't sufficient for production. Consider implementing proper error handling and user feedback.

-  onError={(error) => console.log("Video Error:", error)}
+  onError={(error) => {
+    console.error("Video Error:", error);
+    // Show error state UI
+    setVideoError(index, true);
+    // Optionally retry loading or show fallback content
+  }}

Committable suggestion skipped: line range outside the PR's diff.

/>
);
}
};
Expand Down