Repository navigation
Carousel for web - #30
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request introduces significant modifications to the Changes
Possibly related PRs
Suggested reviewers
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Outside diff range and nitpick comments (8)
components/ui/carousel/render-media-item.tsx (2)
32-34: Consider memoizing styles for better performance.The styles are recalculated on every render. Consider using
useMemoto memoize the styles.const theme = useTheme(); -const styles = stylesFn(theme); +const styles = React.useMemo(() => stylesFn(theme), [theme]);
35-59: Consider using a different resize mode for better image quality.Using
stretchas the resize mode might distort images. Consider usingcoverorcontainfor better image presentation.<Image source={{ uri: item.url || item.uri }} style={[ styles.media, { height: height || 250, width: width || "100%", } ]} - resizeMode="stretch" + resizeMode="cover" />components/ui/carousel/carousel.tsx (4)
25-25: Improve type safety for video references.Consider using a more specific type for video refs:
const videoRefs = useRef<{ [key: number]: { seek: (time: number) => void } }>({});
34-34: Consider extracting magic number to a named constant.The 150ms delay would be clearer as a named constant:
+const SWIPE_RESET_DELAY = 150; // milliseconds -setTimeout(() => setIsSwiping(false), 150); +setTimeout(() => setIsSwiping(false), SWIPE_RESET_DELAY);
55-56: Consider making dimensions responsive or configurable.The hardcoded width and height might not work well across different screen sizes:
const PADDING = 45; const ASPECT_RATIO = 16 / 9; const width = Dimensions.get("window").width - PADDING; const height = width / ASPECT_RATIO; <Carousel width={width} height={height} {...} />
73-73: Add proper typing for indicator mapping.Replace
anywith proper types:-{data.map((_: any, index: any) => ( +{data.map((_: MediaItem, index: number) => (components/InfluencerCard.tsx (2)
132-135: Consider adding error boundaries and loading states.The CarouselNative implementation could benefit from:
- Error handling for failed media loads
- Loading states while media is being fetched
- Fallback UI for empty media arrays
Consider wrapping the carousel with error boundaries and adding loading states:
+ {influencer.media.length === 0 ? ( + <View style={styles.noMediaContainer}> + <Text>No media available</Text> + </View> + ) : ( <CarouselNative data={influencer.media} onImagePress={onImagePress} + onError={(error) => console.error('Carousel error:', error)} + loading={false} // Add loading prop /> + )}
Line range hint
191-208: Enhance modal accessibility.The modal implementation could benefit from accessibility improvements:
- Add appropriate ARIA labels
- Ensure keyboard navigation support
- Add screen reader support for zoom controls
Consider these accessibility enhancements:
<Modal visible={isZoomed} transparent={true} animationType="fade" + accessibilityLabel="Image zoom view" + accessibilityRole="dialog" > <View style={additionalStyles.modalContainer}> <TouchableOpacity style={additionalStyles.closeButton} onPress={() => { setIsZoomed(false); scale.value = 1; }} + accessibilityLabel="Close zoom view" + accessibilityHint="Double tap to close the zoomed image" > <Text style={additionalStyles.closeButtonText}>×</Text> </TouchableOpacity>
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
components/InfluencerCard.tsx(5 hunks)components/ui/carousel/carousel.tsx(1 hunks)components/ui/carousel/carousel.web.tsx(1 hunks)components/ui/carousel/render-media-item.tsx(1 hunks)styles/InfluencerCard.styles.tsx(1 hunks)
🧰 Additional context used
🪛 Biome
components/ui/carousel/carousel.tsx
[error] 36-36: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (6)
styles/InfluencerCard.styles.tsx (1)
62-62: LGTM! Verify layout stability with dynamic content.
The addition of alignSelf: "center" improves the media alignment flexibility. The removal of fixed height (per summary) allows for better content adaptability.
Let's verify the integration with CarouselNative and potential layout impacts:
✅ Verification successful
Based on the search results, I can now generate a final response as we have found important evidence about the media handling:
The alignSelf: "center" change is safe and properly supported
The CarouselNative component and its media rendering are properly handling dimensions:
- The
render-media-item.tsxcomponent explicitly manages both height and width - Default height (250) and width (100%) are provided as fallbacks
- The carousel component itself has proper dimension constraints
The removal of fixed height from InfluencerCard's media style is safe because:
- The parent carousel handles default dimensions
- The media rendering component has built-in dimension management
- The layout stability is maintained through the carousel's structure
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for CarouselNative implementation and related style properties
# Expected: Find CarouselNative component and verify it handles media dimensions
# Search for CarouselNative component implementation
echo "Searching for CarouselNative implementation..."
ast-grep --pattern 'const CarouselNative = $$$'
# Check for any explicit dimension handling in CarouselNative
echo "Checking dimension handling..."
rg -A 5 "CarouselNative.*style.*height|CarouselNative.*style.*width"
# Look for potential layout jump issues in related components
echo "Checking for layout stability measures..."
rg "onLayout|useLayout|measure" --type tsx
Length of output: 460
Script:
#!/bin/bash
# Let's try different approaches to find the CarouselNative component and related styles
# First, let's find all style files and components
echo "Searching for all style and component files..."
fd -e ts -e tsx -e js -e jsx
# Then search for CarouselNative references
echo "Searching for CarouselNative references..."
rg "CarouselNative" -l
# Look for style definitions and dimension handling in any file
echo "Checking style definitions and dimension handling..."
rg "StyleSheet.create.*media|style.*media" -A 5
# Check for layout-related props and handlers
echo "Checking layout handlers..."
rg "onLayout|Layout|measure|height|width" --type ts --type js
Length of output: 18211
components/ui/carousel/render-media-item.tsx (3)
1-14: LGTM! Imports are well-organized and necessary.
All imports are being used appropriately in the component.
24-31: LGTM! Clean component declaration.
The component declaration and props destructuring follow React best practices.
91-91: LGTM! Clean export.
The component export follows standard practices.
components/InfluencerCard.tsx (2)
54-55: LGTM! State management simplified.
The removal of media-related states (currentIndex, muted, isPlaying, isSwiping) and delegation to CarouselNative component improves code maintainability.
26-26: Verify CarouselNative component implementation.
Let's ensure the CarouselNative component exists and is properly implemented.
✅ Verification successful
CarouselNative component is properly implemented
The CarouselNative component exists at the correct location and is properly implemented with:
- A functional React component using react-native-reanimated-carousel
- Proper type definitions and props interface
- Media rendering capabilities through RenderMediaItem
- Carousel controls and state management for images/videos
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if CarouselNative component exists and verify its implementation
fd -e tsx -e ts "carousel.tsx" components/ui/carousel/
Length of output: 90
Script:
#!/bin/bash
# Check the implementation of CarouselNative component
cat components/ui/carousel/carousel.tsx
Length of output: 2244
| const swiperRef = useRef<Swiper>(null); | ||
| const videoRefs = useRef<{ [key: number]: any }>({}); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve ref management and error handling
Several improvements can be made to the component's implementation:
- The
videoRefsare created but there's no cleanup logic - The image press handler lacks error handling
Consider these improvements:
const CarouselWeb: React.FC<CarouselWebProps> = ({
data,
onImagePress,
}) => {
const theme = useTheme();
const styles = stylesFn(theme);
const swiperRef = useRef<Swiper>(null);
const videoRefs = useRef<{ [key: number]: any }>({});
+
+ useEffect(() => {
+ return () => {
+ // Cleanup video refs
+ Object.values(videoRefs.current).forEach(videoRef => {
+ if (videoRef?.pause) {
+ videoRef.pause();
+ }
+ });
+ videoRefs.current = {};
+ };
+ }, []);
const handleImagePress = (uri: string) => {
+ if (!uri) {
+ console.error('Invalid image URI');
+ return;
+ }
if (onImagePress) {
onImagePress(uri);
}
};Also applies to: 22-26
| import React, { useRef } from "react"; | ||
| import Swiper from "react-native-swiper"; | ||
| import stylesFn from "@/styles/tab1.styles"; | ||
| import { useTheme } from "@react-navigation/native"; | ||
| import Colors from "@/constants/Colors"; | ||
| import RenderMediaItem from "./render-media-item"; | ||
|
|
There was a problem hiding this comment.
Review platform-specific dependencies
The component is using react-native-swiper in a web-specific file (.web.tsx). This might cause compatibility issues as React Native components may not work correctly in a web environment.
Consider using a web-specific carousel library like react-slick or swiper.js. Example implementation:
-import Swiper from "react-native-swiper";
+import { Swiper, SwiperSlide } from 'swiper/react';
+import 'swiper/css';Organize styles in a more maintainable way
The styles are imported from tab1.styles, which suggests they might be tightly coupled with a specific tab implementation.
Consider moving carousel-specific styles to a dedicated styles file like carousel.styles.ts for better maintainability and reusability.
Committable suggestion skipped: line range outside the PR's diff.
| interface CarouselWebProps { | ||
| data: any | ||
| onImagePress: (data: any) => void; | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add proper TypeScript types
The interface uses any type which reduces type safety. Define proper types for the data structure and callback parameters.
Consider using more specific types:
+interface MediaItem {
+ id?: string | number;
+ uri: string;
+ type: 'image' | 'video';
+}
interface CarouselWebProps {
- data: any;
- onImagePress: (data: any) => void;
+ data: MediaItem[];
+ onImagePress: (uri: string) => void;
}Committable suggestion skipped: line range outside the PR's diff.
| return ( | ||
| <Swiper | ||
| ref={swiperRef} | ||
| height={560} | ||
| style={styles.wrapper} | ||
| dotStyle={styles.dotStyle} | ||
| activeDotStyle={[ | ||
| styles.dotStyle, | ||
| { | ||
| backgroundColor: Colors(theme).primary, | ||
| }, | ||
| ]} | ||
| paginationStyle={styles.pagination} | ||
| > | ||
| {data.map(( | ||
| item: any, | ||
| index: number, | ||
| ) => ( | ||
| <RenderMediaItem | ||
| handleImagePress={handleImagePress} | ||
| height={560} | ||
| index={index} | ||
| item={item} | ||
| key={item.id || index} | ||
| videoRefs={videoRefs} | ||
| /> | ||
| ))} | ||
| </Swiper> | ||
| ); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance accessibility and responsiveness
The carousel implementation needs improvements in several areas:
- Hardcoded height might not work well across different screen sizes
- Missing accessibility attributes
- No loading or error states
Consider these improvements:
<Swiper
ref={swiperRef}
- height={560}
+ height="100%"
+ style={[styles.wrapper, { minHeight: 300, maxHeight: 560 }]}
style={styles.wrapper}
dotStyle={styles.dotStyle}
+ aria-label="Media carousel"
+ keyboard={true}
activeDotStyle={[
styles.dotStyle,
{
backgroundColor: Colors(theme).primary,
},
]}
paginationStyle={styles.pagination}
>
{data.map((
item: any,
index: number,
) => (
<RenderMediaItem
handleImagePress={handleImagePress}
- height={560}
+ height="100%"
index={index}
item={item}
key={item.id || index}
videoRefs={videoRefs}
+ onError={(error) => console.error(`Failed to load media item ${index}:`, error)}
/>
))}
</Swiper>Committable suggestion skipped: line range outside the PR's diff.
| import React, { useRef } from "react"; | ||
| import Swiper from "react-native-swiper"; | ||
| import stylesFn from "@/styles/tab1.styles"; | ||
| import { useTheme } from "@react-navigation/native"; | ||
| import Colors from "@/constants/Colors"; | ||
| import RenderMediaItem from "./render-media-item"; | ||
|
|
||
| interface CarouselWebProps { | ||
| data: any | ||
| onImagePress: (data: any) => void; | ||
| }; | ||
|
|
||
| const CarouselWeb: React.FC<CarouselWebProps> = ({ | ||
| data, | ||
| onImagePress, | ||
| }) => { | ||
| const theme = useTheme(); | ||
| const styles = stylesFn(theme); | ||
| const swiperRef = useRef<Swiper>(null); | ||
| const videoRefs = useRef<{ [key: number]: any }>({}); | ||
|
|
||
| const handleImagePress = (uri: string) => { | ||
| if (onImagePress) { | ||
| onImagePress(uri); | ||
| } | ||
| }; | ||
|
|
||
| return ( | ||
| <Swiper | ||
| ref={swiperRef} | ||
| height={560} | ||
| style={styles.wrapper} | ||
| dotStyle={styles.dotStyle} | ||
| activeDotStyle={[ | ||
| styles.dotStyle, | ||
| { | ||
| backgroundColor: Colors(theme).primary, | ||
| }, | ||
| ]} | ||
| paginationStyle={styles.pagination} | ||
| > | ||
| {data.map(( | ||
| item: any, | ||
| index: number, | ||
| ) => ( | ||
| <RenderMediaItem | ||
| handleImagePress={handleImagePress} | ||
| height={560} | ||
| index={index} | ||
| item={item} | ||
| key={item.id || index} | ||
| videoRefs={videoRefs} | ||
| /> | ||
| ))} | ||
| </Swiper> | ||
| ); | ||
| }; | ||
|
|
||
| export default CarouselWeb; |
There was a problem hiding this comment.
💡 Codebase verification
Platform-specific carousel implementation needs adjustment
The verification revealed several issues that need attention:
- The web carousel imports styles from
@/styles/tab1.styleswhich doesn't exist in the expected locations - The InfluencerCard component currently uses CarouselNative but lacks platform-specific handling for web
- The styles referenced in CarouselWeb (
wrapper,pagination,dotStyle) are not properly defined
Required changes:
- Move styles to a shared location or create platform-specific style files
- Add platform-specific import logic in InfluencerCard to use CarouselWeb for web platform
- Ensure style definitions are available and consistent across platforms
🔗 Analysis chain
Verify integration with InfluencerCard component
Since this is a new web-specific carousel implementation, ensure it integrates correctly with the parent component.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check usage of CarouselWeb in InfluencerCard
rg -l "CarouselWeb" --type tsx
# Check for any existing carousel implementations that might conflict
rg -l "Carousel" --type tsx
# Check for responsive design implementations
ast-grep --pattern 'style={[^}]*height'
Length of output: 156
Script:
#!/bin/bash
# Check usage of CarouselWeb component
rg -l "CarouselWeb"
# Check for any existing carousel implementations
rg -l "Carousel"
# Check the parent component's integration
rg -A 5 "import.*CarouselWeb"
# Look for style definitions that might affect responsive design
rg "wrapper|pagination|dotStyle" tab1.styles
Length of output: 313
Script:
#!/bin/bash
# Check how CarouselWeb is used in InfluencerCard
rg -A 10 "CarouselWeb" components/InfluencerCard.tsx
# Check the base carousel implementation
cat components/ui/carousel/carousel.tsx
# Look for style definitions in the correct path
rg "wrapper|pagination|dotStyle" components/ui/carousel/
Length of output: 2645
Script:
#!/bin/bash
# Check the styles import path
cat components/styles/tab1.styles.ts 2>/dev/null || cat styles/tab1.styles.ts 2>/dev/null
# Check if there are any platform-specific imports or conditionals
rg -A 5 "Platform.select|Platform.OS" components/ui/carousel/
# Check the actual usage in InfluencerCard to see platform handling
rg -B 5 -A 15 "import.*Carousel" components/InfluencerCard.tsx
Length of output: 1185
| return ( | ||
| <Video | ||
| ref={(ref) => { | ||
| if (ref && videoRefs) { | ||
| videoRefs.current[index] = ref; | ||
| } | ||
| }} | ||
| source={ | ||
| item.appleUrl | ||
| ? { | ||
| uri: Platform.OS === "ios" ? item.appleUrl : item.playUrl, | ||
| } | ||
| : require("@/assets/videos/ForBiggerJoyrides.mp4") | ||
| } | ||
| style={[ | ||
| styles.media, | ||
| { | ||
| height: height || 250, | ||
| width: width || "100%", | ||
| } | ||
| ]} | ||
| resizeMode={ResizeMode.COVER} | ||
| isLooping={false} | ||
| shouldPlay={false} | ||
| useNativeControls | ||
| onError={(error) => console.log("Video Error:", error)} | ||
| /> | ||
| ); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Several improvements needed in video handling.
- The hardcoded fallback video path creates a tight coupling. Consider making it configurable.
- Error handling should use proper error logging/reporting.
- Video ref type should be properly typed.
+import { Video as ExpoVideo } from 'expo-av';
+
+interface VideoRefs {
+ [key: number]: ExpoVideo | null;
+}
interface RenderMediaItemProps {
// ...
- videoRefs?: React.MutableRefObject<{ [key: number]: any }>;
+ videoRefs?: React.MutableRefObject<VideoRefs>;
// ...
}
// Later in the code:
source={
item.appleUrl
? {
uri: Platform.OS === "ios" ? item.appleUrl : item.playUrl,
}
- : require("@/assets/videos/ForBiggerJoyrides.mp4")
+ : props.fallbackVideo // Add this to props
}
// ...
- onError={(error) => console.log("Video Error:", error)}
+ onError={(error) => {
+ // Use your app's error reporting mechanism
+ reportError('Video playback error', error);
+ }}Committable suggestion skipped: line range outside the PR's diff.
| interface RenderMediaItemProps { | ||
| handleImagePress: (uri: string) => void; | ||
| height?: number; | ||
| index: number; | ||
| item: any; | ||
| videoRefs?: React.MutableRefObject<{ [key: number]: any }>; | ||
| width?: number; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Define proper types for the media item instead of using 'any'.
Using any type reduces type safety. Consider creating a proper type definition for the media item.
+interface MediaItem {
+ type: 'image' | 'video';
+ uri?: string;
+ url?: string;
+ appleUrl?: string;
+ playUrl?: string;
+}
interface RenderMediaItemProps {
handleImagePress: (uri: string) => void;
height?: number;
index: number;
- item: any;
+ item: MediaItem;
videoRefs?: React.MutableRefObject<{ [key: number]: any }>;
width?: number;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| interface RenderMediaItemProps { | |
| handleImagePress: (uri: string) => void; | |
| height?: number; | |
| index: number; | |
| item: any; | |
| videoRefs?: React.MutableRefObject<{ [key: number]: any }>; | |
| width?: number; | |
| } | |
| interface MediaItem { | |
| type: 'image' | 'video'; | |
| uri?: string; | |
| url?: string; | |
| appleUrl?: string; | |
| playUrl?: string; | |
| } | |
| interface RenderMediaItemProps { | |
| handleImagePress: (uri: string) => void; | |
| height?: number; | |
| index: number; | |
| item: MediaItem; | |
| videoRefs?: React.MutableRefObject<{ [key: number]: any }>; | |
| width?: number; | |
| } |
| interface CarouselProps { | ||
| data: any[]; // TODO: Add specific type | ||
| onImagePress?: (data: any) => void; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace any type with a proper interface definition.
The any type reduces type safety. Based on the component's usage, consider defining a proper type:
interface MediaItem {
uri: string;
type: 'image' | 'video';
thumbnail?: string;
}
interface CarouselProps {
data: MediaItem[];
onImagePress?: (uri: string) => void;
}| Object.values(videoRefs.current).forEach((videoRef) => { | ||
| if (videoRef && videoRef.seek) { | ||
| videoRef.seek(0); | ||
| setIsPlaying(false); | ||
| } | ||
| }); | ||
| }; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve video ref handling with optional chaining and cleanup.
The video ref access could be safer and should be cleaned up on unmount:
// Inside component:
useEffect(() => {
return () => {
// Cleanup video refs on unmount
Object.values(videoRefs.current).forEach((videoRef) => {
videoRef?.seek?.(0);
});
};
}, []);
// In handleIndexChange:
Object.values(videoRefs.current).forEach((videoRef) => {
videoRef?.seek?.(0);
setIsPlaying(false);
});🧰 Tools
🪛 Biome
[error] 36-36: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
| const onImagePress = (data: any) => { | ||
| setSelectedImage(data); // TODO: data -> uri | ||
| setIsZoomed(true); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve type safety in onImagePress handler.
The any type and TODO comment suggest room for improvement:
- The parameter type should be more specific
- The TODO comment should be addressed
Consider this improvement:
- const onImagePress = (data: any) => {
- setSelectedImage(data); // TODO: data -> uri
+ const onImagePress = (uri: string) => {
+ setSelectedImage(uri);Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
components/InfluencerCard.tsx (1)
148-151: Add error handling for media loading failuresThe carousel implementation lacks error boundaries and fallback UI for media loading failures. This could lead to a poor user experience if media fails to load.
Consider adding error handling:
<CarouselNative data={influencer.media} onImagePress={onImagePress} + onError={(error) => console.error('Media loading failed:', error)} + fallbackComponent={ + <View style={styles.mediaFallback}> + <Text>Failed to load media</Text> + </View> + } />
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
components/InfluencerCard.tsx(5 hunks)
🔇 Additional comments (1)
components/InfluencerCard.tsx (1)
27-27: Verify CarouselNative component implementation
The import suggests a local carousel component. Let's verify its implementation and ensure it handles all the media types properly.
✅ Verification successful
CarouselNative component is properly implemented with platform-specific handling
The import in InfluencerCard.tsx is correct and the component exists with proper implementation:
- Native version uses
react-native-reanimated-carousel - Web version uses
react-native-swiper - Both versions handle media items through a shared
RenderMediaItemcomponent - Proper handling of image press events and video refs
- Platform-specific styling and dimensions
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if the CarouselNative component exists and verify its implementation
ast-grep --pattern 'export $_ function CarouselNative' ./components/ui/carousel/carousel.tsx
# Check for media type handling in the component
rg -A 10 'type.*=.*string' ./components/ui/carousel/carousel.tsx
Length of output: 161
Script:
#!/bin/bash
# Let's try different approaches to locate the carousel component
# First, let's check if the file exists with different extensions
fd "carousel" components/ui/carousel/
# If not found in that specific path, search more broadly for carousel-related files
fd "carousel" components/
# Look for any exports containing "Carousel" in case the component name is different
rg -l "export.*Carousel" components/
# Search for carousel-related imports to trace its usage
rg -l "import.*[Cc]arousel" components/
Length of output: 462
Script:
#!/bin/bash
# Let's examine the carousel implementation
cat components/ui/carousel/carousel.tsx
# Also check the web version to understand the platform-specific implementation
cat components/ui/carousel/carousel.web.tsx
Length of output: 3648
| useEffect(() => { | ||
| const timer = setTimeout(() => { | ||
| setLoading(false); | ||
| }, 1000); | ||
|
|
||
| const renderMediaItem = ({ item, index }: any) => { | ||
| if (item.type === "image") { | ||
| return ( | ||
| <TapGestureHandler | ||
| onHandlerStateChange={({ nativeEvent }) => { | ||
| if (nativeEvent.state === State.ACTIVE) { | ||
| handleImagePress(item.uri); | ||
| } | ||
| }} | ||
| > | ||
| <Animated.View> | ||
| <Image | ||
| source={{ uri: item.url || item.uri }} | ||
| style={styles.media} | ||
| /> | ||
| </Animated.View> | ||
| </TapGestureHandler> | ||
| ); | ||
| } else { | ||
| return ( | ||
| <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={false} | ||
| shouldPlay={false} | ||
| useNativeControls | ||
| onError={(error) => console.log("Video Error:", error)} | ||
| /> | ||
| ); | ||
| } | ||
| }; | ||
| return () => { | ||
| clearTimeout(timer); | ||
| }; | ||
| }, []); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve loading state implementation
The current implementation uses a fixed 1-second timeout which might not reflect actual media loading times. This could lead to:
- Showing loading spinner even after content is ready
- Hiding loading spinner before content is fully loaded
Consider implementing actual loading state tracking:
- const [loading, setLoading] = useState(true);
+ const [mediaLoading, setMediaLoading] = useState(true);
- useEffect(() => {
- const timer = setTimeout(() => {
- setLoading(false);
- }, 1000);
-
- return () => {
- clearTimeout(timer);
- };
- }, []);
-
- if (loading) {
+ if (mediaLoading) {
return <ActivityIndicator />;
}Then pass the loading state handler to CarouselNative:
<CarouselNative
data={influencer.media}
onImagePress={onImagePress}
+ onLoadComplete={() => setMediaLoading(false)}
/>Also applies to: 111-113
Summary by CodeRabbit
Release Notes
New Features
CarouselNativeandCarouselWebcomponents for enhanced media display.RenderMediaItemcomponent to handle rendering of images and videos.Improvements
InfluencerCardcomponent, simplifying state management and rendering logic.ActivityIndicatorfor improved user feedback during media loading.DrawerMenuContentto enhance layout and usability.These changes enhance the user experience by providing a more intuitive and visually appealing media display.