-
Notifications
You must be signed in to change notification settings - Fork 0
Implement ProfileHeader
Composable
#18
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: trunk
Are you sure you want to change the base?
Conversation
3a3e08a
to
83cf215
Compare
|
||
@Composable | ||
fun ProfileHeader( | ||
profile: Profile, |
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.
I'm using the Profile
class from the Gravatar-SDK. If that were from a third-party library, it would be a good idea to map that object to another one managed by the app. In this case, since we are using our library, I consider it under our control.
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.
Yeah, I have mixed feelings about this 😬, but the components expect the same models so it is how it is.
83cf215
to
4a90a49
Compare
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.
Pull Request Overview
This PR implements the ProfileHeader Composable and integrates the Gravatar SDK to fetch and display user profile data. Key changes include the addition of tests for various ProfileHeader states, updates to the ProfileViewModel to fetch profiles (with a temporary hardcoded username), and adjustments to dependency injection along with updated build configurations.
Reviewed Changes
Copilot reviewed 9 out of 14 changed files in this pull request and generated 1 comment.
Show a summary per file
File | Description |
---|---|
ProfileHeaderTest.kt | Added screenshot tests covering different combinations of job title and company. |
HomeScreenTest.kt | Updated package and test name to reflect the home screen context. |
ProfileViewModel.kt | Introduced a ViewModel to fetch the profile using the Gravatar SDK with a temporary hardcoded username. |
ProfileUiState.kt | Defined a new data state to represent profile loading and data. |
ProfileScreen.kt | Updated UI to conditionally display the ProfileHeader or an error message based on loading state. |
ProfileHeader.kt | Implemented the ProfileHeader Composable utilizing the Avatar component and handling job info display. |
HomeUiModule.kt | Configured dependency injection for ProfileService and ProfileViewModel. |
build.gradle.kts & libs.versions.toml | Added Gravatar and Koin dependencies to support the new functionality. |
Comments suppressed due to low confidence (1)
homeUi/src/main/kotlin/com/gravatar/app/homeUi/presentation/home/profile/ProfileScreen.kt:30
- [nitpick] Consider refining the error message to be more user-friendly or to provide actionable guidance, such as suggesting a retry option.
Text("There was an error retrieving the profile.")
fetchProfile() | ||
} | ||
|
||
private fun fetchProfile() { | ||
viewModelScope.launch { | ||
_uiState.update { currentState -> currentState.copy(isLoading = true) } | ||
// -> Remove the hardcoded username <- | ||
when (val result = profileService.retrieveCatching("hamorillo")) { |
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.
Consider removing the hardcoded username 'hamorillo' and parameterizing the profile retrieval to better support dynamic user data once OAuth integration is complete.
fetchProfile() | |
} | |
private fun fetchProfile() { | |
viewModelScope.launch { | |
_uiState.update { currentState -> currentState.copy(isLoading = true) } | |
// -> Remove the hardcoded username <- | |
when (val result = profileService.retrieveCatching("hamorillo")) { | |
// Replace with a dynamic username once OAuth or user session management is integrated | |
fetchProfile("hamorillo") | |
} | |
private fun fetchProfile(username: String) { | |
viewModelScope.launch { | |
_uiState.update { currentState -> currentState.copy(isLoading = true) } | |
when (val result = profileService.retrieveCatching(username)) { |
Copilot uses AI. Check for mistakes.
We are implementing a basic ViewModel for the ProfileScreen using Gravatar-Core to load a profile.
4a90a49
to
439e598
Compare
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.
Looks good 👍 I've left some comments, but none are breaking.
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.
💡 We can set a fake image loader for the screenshot test like we did in the SDK - https://github.com/Automattic/Gravatar-SDK-Android/blob/trunk/uitestutils/src/main/java/com/gravatar/uitestutils/RoborazziTest.kt#L60
Not a big deal, but it helps visualize the full component better.
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.
Yeah, I was considering this, but if you don't mind, I'll open a different PR with it.
import com.gravatar.ui.components.atomic.Avatar | ||
|
||
@Composable | ||
fun ProfileHeader( |
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.
⛏️ Missing internal
modifier
|
||
@Composable | ||
fun ProfileScreen() { | ||
fun ProfileScreen(viewModel: ProfileViewModel = koinViewModel()) { |
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.
We can make this internal
as well.
Box( | ||
modifier = Modifier.fillMaxSize(), | ||
contentAlignment = Alignment.Center | ||
) { | ||
Text("Profile Screen") | ||
if (uiState.isLoading) { | ||
CircularProgressIndicator(modifier = Modifier.align(Alignment.Center)) | ||
} else { | ||
uiState.profile.let { profile -> | ||
if (profile != null) { | ||
ProfileHeader(profile = profile, modifier = Modifier.padding(16.dp)) | ||
} else { | ||
Text("There was an error retrieving the profile.") | ||
} | ||
} | ||
} | ||
} |
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.
💡 I think we should extract this to a new ProfileScreen(uiState: ProfileUiState)
component - otherwise it's hard to create previews or run screenshot tests for the whole screen.
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.
Done! Also added a screenshot test.
Column { | ||
Text( | ||
text = profile.displayName, | ||
style = MaterialTheme.typography.titleMedium, | ||
fontWeight = FontWeight.Bold | ||
) | ||
|
||
profile.jobInfo().takeIf { it.isNotBlank() }?.let { jobInfo -> | ||
Text( | ||
text = jobInfo, | ||
style = MaterialTheme.typography.bodyMedium, | ||
color = MaterialTheme.colorScheme.onSurface.copy(alpha = 0.6f) | ||
) | ||
} | ||
} |
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.
💡 We have a similar problem here like in the SDK - the card doesn't look good when you start scaling the font size. What if we used the autoSize
and set those to single lines? Something like:
BasicText(
text = profile.displayName,
style = MaterialTheme.typography.titleMedium,
maxLines = 1,
autoSize = TextAutoSize.StepBased(
maxFontSize = 16.sp
)
)
It needs some styling but here's the difference with the max font scaling
Before | After |
---|---|
![]() |
![]() |
} else { | ||
uiState.profile.let { profile -> | ||
if (profile != null) { | ||
ProfileHeader(profile = profile, modifier = Modifier.padding(16.dp)) |
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.
❓ This will have to be a top bar. I wonder if we will need a scaffold here as well, or we will just keep it at the top? Have you thought about this?
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.
I haven't thought much about it, as we'll implement the animation to collapse/expand the header, and that can require some changes. My idea was to keep it at the top for now.
|
||
@Composable | ||
fun ProfileHeader( | ||
profile: Profile, |
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.
Yeah, I have mixed feelings about this 😬, but the components expect the same models so it is how it is.
Description
The main goal of this PR is to implement the ProfileHeader Composable. We need to add the Gravatar SDK dependencies for two reasons:
Avatar
Composable)Note: I've hardcoded my user for the moment. Once the OAuth tasks are complete, we'll need to use that information to fetch the profile.
Testing Steps
ProfileHeader
in the Profile section.