Conversation
There was a problem hiding this comment.
Pull Request Overview
This pull request implements a UI enhancement feature that improves the visual design and user experience across multiple screens and components. The changes include redesigned profile headers, improved icon theming, better screen layouts, and various UI refinements.
- Redesigned profile header components with improved layout and styling
- Added new ThemedIcon component with color-coded icons throughout settings screens
- Enhanced screen layouts with better spacing, padding, and visual hierarchy
- Improved back button styling and various UI component refinements
Reviewed Changes
Copilot reviewed 29 out of 29 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| UserFields.kt | Converted to ColumnScope extension and removed card wrapper styling |
| ProfileHeader.kt | Removed Column wrapper from profile header content area |
| CommonProfileHeader.kt | Added conditional styling for big screen vs mobile layouts |
| StatusDetailRemoteMediator.kt | Simplified tweet detail fetching logic and removed complex ID handling |
| FollowingPagingSource.kt | Added filtering for specific card types in VVO data processing |
| FansPagingSource.kt | Added filtering for specific card types in VVO data processing |
| ThemedIcon.kt | New component providing color-coded icons with themed backgrounds |
| Various settings screens | Updated to use ThemedIcon component and improved layouts |
| Various screen files | Added back button styling and improved navigation patterns |
| internal fun ColumnScope.UserFields(fields: ImmutableMap<String, UiRichText>) { | ||
| fields.onEachIndexed { index, (key, value) -> | ||
| UserField( | ||
| key = key, | ||
| value = value, | ||
| ) |
There was a problem hiding this comment.
The function signature change from accepting a Modifier parameter to being a ColumnScope extension removes the ability to apply custom modifiers. This is a breaking API change that may affect existing callers who need to apply styling.
| is TweetWithVisibilityResults -> | ||
| result.tweet.legacy?.conversationIdStr == statusKey.id | ||
| null -> false | ||
| else -> true |
There was a problem hiding this comment.
The catch-all 'else -> true' condition may include unintended tweet types in the results. Consider being more explicit about which types should be included to avoid potential issues with future API changes.
| else -> true | |
| is KnownTweetType1 -> true // Replace with actual known tweet type | |
| is KnownTweetType2 -> true // Replace with actual known tweet type | |
| else -> { | |
| // Log a warning or handle unknown types explicitly | |
| false | |
| } |
| } else { | ||
| it.dark | ||
| } | ||
| } ?: error("Unknown color: $color") |
There was a problem hiding this comment.
The error message should be more descriptive and helpful for debugging. Consider including the actual color value that was not found.
| } ?: error("Unknown color: $color") | |
| } ?: error("Unknown color: $color. Available colors: ${iconColorData.keys}") |
| ) | ||
| }, | ||
| overlineContent = { | ||
| Text( |
There was a problem hiding this comment.
The hex color conversion logic 'color.toArgb().toHexString().uppercase()' appears to use an extension function 'toHexString()' that is not imported or defined in the visible imports. This may cause compilation errors.
| .let { | ||
| if (isBigScreen()) { | ||
| it | ||
| } else { | ||
| it | ||
| .padding(horizontal = screenHorizontalPadding) | ||
| .padding(bottom = 8.dp) | ||
| .listCard() | ||
| .background(PlatformTheme.colorScheme.card) | ||
| .padding(horizontal = screenHorizontalPadding, vertical = 8.dp) | ||
| .fillMaxWidth() | ||
| } | ||
| }, |
There was a problem hiding this comment.
[nitpick] The nested conditional styling logic is complex and difficult to read. Consider extracting this into a separate modifier extension function for better maintainability.
| .let { | |
| if (isBigScreen()) { | |
| it | |
| } else { | |
| it | |
| .padding(horizontal = screenHorizontalPadding) | |
| .padding(bottom = 8.dp) | |
| .listCard() | |
| .background(PlatformTheme.colorScheme.card) | |
| .padding(horizontal = screenHorizontalPadding, vertical = 8.dp) | |
| .fillMaxWidth() | |
| } | |
| }, | |
| .applyProfileHeaderModifiers(), |
No description provided.