-
Notifications
You must be signed in to change notification settings - Fork 32
DemoApp - First interation #42
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
Conversation
Using the Gravatar library to generate the GravatarUrl that we use to load the image.
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.
LGTM for a first version.
We could do some minor changes, IMO most import is the theming stuff (it should become an habit to check and pick dynamic theme colors).
} | ||
|
||
@Composable | ||
fun GravatarImage(gravatarUrl: String) = AsyncImage(model = gravatarUrl, contentDescription = null) |
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 go ahead and log something onError
here. This might be useful for testing.
And I wonder if we should show something specific when we get a 404 (eg. with the "Default avatar: 404" selected)
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 agree; I've created a new ticket, #44, to create a PR with the error handling.
fun GravatarImageSettingsPreview() { | ||
GravatarDemoAppTheme { | ||
GravatarImageSettings( | ||
email = "[email protected]", |
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.
Can we use a test email that has a gravatar image? This will simplify testing. There must be some demo account, if not we should create some.
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.
Using [email protected]
now. It was mentioned here.
Column( | ||
modifier = Modifier | ||
.fillMaxSize() | ||
.background(MaterialTheme.colorScheme.background) |
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.
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.
onEmailChanged = { email = it }, | ||
onSizeChange = { avatarSize = it }, | ||
onLoadGravatarClicked = { | ||
gravatarUrl = emailAddressToGravatarUrl( |
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 wonder if we should:
- catch the illegal argument exceptions here
- limit the size input to be between 1..2048 in the text field component
- or sanitize size input in the text field component
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.
Given this is a demo/test app, I think we should go with the first one and show the error in the UI. I'll work on this in #44.
❤️ Great suggestions!
Adding a surface as a parent composable makes the inside composables use the surface color when needed.
#26
This PR is the iteration to create a DemoApp that uses the GravatarUtils we support in the SDK. The app allows you to introduce an e-mail for loading its' corresponding GravatarImageUrl.
In addition, you can play with the params "Default Avatar Image" and "Avatar Size".
Next Steps: Include the rest of the possible parameters and polish the app.
Kapture.2024-01-26.at.12.52.27.mp4