-
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
fix: Avatar styles #1486
fix: Avatar styles #1486
Conversation
WalkthroughThe pull request introduces changes across multiple components, primarily focusing on the navigation logic and rendering behavior. The Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (5)
⏰ Context from checks skipped due to timeout of 90000ms (1)
🔇 Additional comments (8)
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 |
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)
components/Avatar.tsx (1)
Line range hint
50-61
: Consider standardizing the style application approach.For consistency with the placeholder implementation, consider explicitly setting size and borderRadius on the Image wrapper View:
- <View style={style}> + <View + style={StyleSheet.flatten([ + style, + { width: size, height: size, borderRadius: size / 2 }, + ])} + >This ensures consistent sizing behavior between image and placeholder states.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
components/Avatar.tsx
(1 hunks)
🔇 Additional comments (1)
components/Avatar.tsx (1)
Line range hint
50-61
: Verify layout consistency across different style configurations.The removal of
styles.imageContainer
creates an asymmetry between image and placeholder styling. While the Image component maintains its dimensions throughstyles.image
, the View wrapper now relies solely on external styling.Please verify that the avatar maintains proper layout and alignment in these scenarios:
- Without any external style prop
- With margin/padding in the style prop
- Within flex containers
Run this script to find avatar usage patterns:
✅ Verification successful
Style changes maintain layout consistency across the codebase
The removal of
styles.imageContainer
is safe as the Avatar component:
- Maintains proper layout through the parent View's style prop
- Preserves size constraints via
styles.image
- Shows consistent usage across various layout contexts (flex, fixed, etc.)
- Successfully integrates with the theme's sizing system
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for Avatar component usage patterns rg -t tsx -t jsx "Avatar" -A 3 -B 1Length of output: 63
Script:
#!/bin/bash # Find TypeScript/React files containing Avatar fd -e ts -e tsx -e jsx | xargs rg "Avatar" -l # Show Avatar imports and usages with context fd -e ts -e tsx -e jsx | xargs rg "Avatar" -B 2 -A 3 # Look specifically for style props passed to Avatar fd -e ts -e tsx -e jsx | xargs rg "<Avatar.*style" -B 1 -A 2Length of output: 79911
Updates to work with Navigation v7
@@ -47,7 +47,7 @@ function Avatar({ | |||
setDidError(false); | |||
}, []); | |||
return uri && !didError ? ( | |||
<View style={[styles.imageContainer, style]}> | |||
<View style={style}> |
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.
The style
prop is being applied to a wrapper View
that no longer has width/height constraints. This could lead to incorrect sizing of the avatar. Consider either:
- Moving the dimension styles back to an
imageContainer
style object, or - Removing the
View
wrapper and applyingstyle
directly to theImage
component
The second option would reduce nesting and simplify the component structure.
Spotted by Graphite Reviewer
Is this helpful? React 👍 or 👎 to let us know.
With Expo 52 upgrade I removed an unnecessary Image Background (updated designs didn't need it) for Image but didn't update the styles correctly to set a height/width
Summary by CodeRabbit