-
Notifications
You must be signed in to change notification settings - Fork 26
[jules] ux: Complete skeleton loading for HomeScreen groups #323
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,36 @@ | ||||||||||||||||||||||
| import React from 'react'; | ||||||||||||||||||||||
| import { View, StyleSheet } from 'react-native'; | ||||||||||||||||||||||
| import { Card } from 'react-native-paper'; | ||||||||||||||||||||||
| import Skeleton from '../ui/Skeleton'; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| const HomeScreenSkeleton = () => { | ||||||||||||||||||||||
| return ( | ||||||||||||||||||||||
| <View style={styles.container} accessibilityRole="progressbar" accessibilityLabel="Loading groups"> | ||||||||||||||||||||||
| {Array.from({ length: 5 }).map((_, i) => ( | ||||||||||||||||||||||
| <Card key={i} style={styles.card}> | ||||||||||||||||||||||
| <Card.Title | ||||||||||||||||||||||
| title={<Skeleton width={120} height={18} borderRadius={4} />} | ||||||||||||||||||||||
| left={() => <Skeleton width={40} height={40} borderRadius={20} />} | ||||||||||||||||||||||
| /> | ||||||||||||||||||||||
|
Comment on lines
+10
to
+14
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🧩 Analysis chain🌐 Web query:
💡 Result: The title prop of Card.Title in react-native-paper 5.x accepts React.ReactNode values. This includes strings, numbers, JSX elements (like components), and other renderable React children. The TypeScript definition explicitly states: "Text for the title. Note that this will only accept a string or -based node," confirming compatibility with text-based React nodes. Citations:
🏁 Script executed: # First, check if the file exists and verify the code snippet
cat -n mobile/components/skeletons/HomeScreenSkeleton.js | head -20Repository: Devasy/splitwiser Length of output: 975 🏁 Script executed: # Find react-native-paper package.json to identify version
find . -name "package.json" -type f | head -5 | xargs grep -l "react-native-paper" 2>/dev/nullRepository: Devasy/splitwiser Length of output: 82 🏁 Script executed: # Check for Skeleton component definition in the repo
fd -t f "Skeleton" --extension js --extension ts --extension tsx mobile/Repository: Devasy/splitwiser Length of output: 143 🏁 Script executed: # Look for HomeScreen.js file to check the pattern at line 196
fd -t f "HomeScreen.js"Repository: Devasy/splitwiser Length of output: 89 🏁 Script executed: # Check the react-native-paper version
cat mobile/package.json | grep -A 2 -B 2 "react-native-paper"Repository: Devasy/splitwiser Length of output: 241 🏁 Script executed: # Examine the Skeleton component definition
cat mobile/components/ui/Skeleton.jsRepository: Devasy/splitwiser Length of output: 1096 🏁 Script executed: # Check HomeScreen.js line 196 and surrounding context
sed -n '190,205p' mobile/screens/HomeScreen.jsRepository: Devasy/splitwiser Length of output: 646 🏁 Script executed: # Check if node_modules has react-native-paper TypeScript definitions
find node_modules -path "*/react-native-paper/*/CardTitle.d.ts" 2>/dev/null | head -1 | xargs catRepository: Devasy/splitwiser Length of output: 43 🏁 Script executed: # Alternative: search for CardTitle types in node_modules
find node_modules/react-native-paper -name "*Card*" -type f | grep -E "\.(d\.ts|ts)$" | head -5Repository: Devasy/splitwiser Length of output: 127 🏁 Script executed: # Let's also check what HapticCard is in the codebase (different from Card.Title pattern)
rg -t ts -t tsx "HapticCard" mobile/ -B 2 -A 2 | head -30Repository: Devasy/splitwiser Length of output: 88 🏁 Script executed: # Let's search for how Card.Title is used elsewhere in the codebase
rg "Card\.Title" mobile/ -B 2 -A 4 -t jsRepository: Devasy/splitwiser Length of output: 4271 🏁 Script executed: # Check if there's a type definition file in the project itself
find . -name "*.d.ts" -type f | head -10Repository: Devasy/splitwiser Length of output: 43 🏁 Script executed: # Verify if there are any runtime issues or patterns in similar React Native projects
# Let's also check if the title prop is documented to accept render functions
rg -t js "title=\{.*=>.*\}" mobile/ | head -10Repository: Devasy/splitwiser Length of output: 43 🏁 Script executed: # Double-check if there's any other use of JSX elements as Card.Title props
rg "Card\.Title" mobile/ -A 2 | grep -E "title=\{<"Repository: Devasy/splitwiser Length of output: 184
🔧 Proposed fix <Card.Title
- title={<Skeleton width={120} height={18} borderRadius={4} />}
+ title={() => <Skeleton width={120} height={18} borderRadius={4} />}
left={() => <Skeleton width={40} height={40} borderRadius={20} />}
/>📝 Committable suggestion
Suggested change
🤖 Prompt for AI Agents |
||||||||||||||||||||||
| <Card.Content> | ||||||||||||||||||||||
| <Skeleton width={180} height={14} borderRadius={4} style={styles.contentSkeleton} /> | ||||||||||||||||||||||
| </Card.Content> | ||||||||||||||||||||||
| </Card> | ||||||||||||||||||||||
| ))} | ||||||||||||||||||||||
| </View> | ||||||||||||||||||||||
| ); | ||||||||||||||||||||||
| }; | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| const styles = StyleSheet.create({ | ||||||||||||||||||||||
| container: { | ||||||||||||||||||||||
| padding: 16, | ||||||||||||||||||||||
| }, | ||||||||||||||||||||||
| card: { | ||||||||||||||||||||||
| marginBottom: 16, | ||||||||||||||||||||||
| }, | ||||||||||||||||||||||
| contentSkeleton: { | ||||||||||||||||||||||
| marginTop: 4, | ||||||||||||||||||||||
| }, | ||||||||||||||||||||||
| }); | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| export default HomeScreenSkeleton; | ||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,45 @@ | ||
| import React, { useEffect, useRef } from 'react'; | ||
| import { Animated, View, StyleSheet } from 'react-native'; | ||
| import { useTheme } from 'react-native-paper'; | ||
|
|
||
| const Skeleton = ({ width, height, borderRadius = 4, style }) => { | ||
| const theme = useTheme(); | ||
| const opacityAnim = useRef(new Animated.Value(0.3)).current; | ||
|
|
||
| useEffect(() => { | ||
| const loop = Animated.loop( | ||
| Animated.sequence([ | ||
| Animated.timing(opacityAnim, { | ||
| toValue: 1, | ||
| duration: 700, | ||
| useNativeDriver: true, | ||
| }), | ||
| Animated.timing(opacityAnim, { | ||
| toValue: 0.3, | ||
| duration: 700, | ||
| useNativeDriver: true, | ||
| }), | ||
| ]) | ||
| ); | ||
| loop.start(); | ||
|
|
||
| return () => loop.stop(); | ||
| }, [opacityAnim]); | ||
|
|
||
| return ( | ||
| <Animated.View | ||
| style={[ | ||
| { | ||
| width, | ||
| height, | ||
| borderRadius, | ||
| backgroundColor: theme.colors.surfaceVariant || '#e0e0e0', | ||
| opacity: opacityAnim, | ||
| }, | ||
| style, | ||
| ]} | ||
| /> | ||
| ); | ||
| }; | ||
|
|
||
| export default Skeleton; |
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.
markdownlint MD022: add a blank line after the
### Mobileheading.📝 Proposed fix
### Mobile + - Added reusable animated `Skeleton` primitive component in `mobile/components/ui/Skeleton.js`.📝 Committable suggestion
🧰 Tools
🪛 markdownlint-cli2 (0.22.0)
[warning] 127-127: Headings should be surrounded by blank lines
Expected: 1; Actual: 0; Below
(MD022, blanks-around-headings)
🤖 Prompt for AI Agents