Error and Loading State UI #33
Conversation
…my/error-loading-states
zachseidner1
left a comment
zachseidner1
left a comment
There was a problem hiding this comment.
Nice work Amy! There's some improvements to be made, and I'd love for you to implement the shimmer modifier extension function if you have time. But things look good!
app/src/main/java/com/cornellappdev/score/components/ErrorState.kt
Outdated
Show resolved
Hide resolved
| verticalArrangement = Arrangement.SpaceBetween | ||
| ) { | ||
| Column( | ||
| horizontalAlignment = Alignment.CenterHorizontally |
There was a problem hiding this comment.
Redundant parameter here
There was a problem hiding this comment.
i made a column for the feedback icon + text and nested that + the button within the main column, so i think both of these are necessary
app/src/main/java/com/cornellappdev/score/components/ErrorState.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/cornellappdev/score/components/ErrorState.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/cornellappdev/score/components/LoadingStateBox.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/cornellappdev/score/components/LoadingState.kt
Outdated
Show resolved
Hide resolved
| import com.cornellappdev.score.theme.GrayStroke | ||
|
|
||
| @Composable | ||
| fun LoadingStateBox(cornerRoundness: Int, height: Dp) { |
There was a problem hiding this comment.
Also I think it's better to just expose a Modifier parameter here instead of 2 separate parameters
There was a problem hiding this comment.
since the cornerRoundness is nested within Modifier.background and I'm passing in the same color parameter each time for Modifier.background, is it okay if i keep the cornerRoundness so I can pass it in and use a modifier parameter for the height/width?
There was a problem hiding this comment.
i also want to have the default width be the maxWidth and I don't have a default value for the height so maybe it would be better to leave height as it's own parameter too?
zachseidner1
left a comment
zachseidner1
left a comment
There was a problem hiding this comment.
Please take a look at my comments, I'd like you to fix previews and establish a ScorePreview component for use down the line. I trust you to make these changes well so I am approving this now so you can merge after making those changes. Nice work!
| @Preview | ||
| @Composable | ||
| private fun GameDetailsLoadingStatePreview() { | ||
| GameDetailsLoadingState() |
There was a problem hiding this comment.
So because you used a Composition local provider, this breaks your previews since we didn't provide the value here. Could you make a custom preview function ScorePreview that is sort of like ResellPreview, and then provide the value through a custom ScoreTheme function? Sorry to have you just using the Resell code so much but I do think it's the best solution. I don't want you to just feel like you're copying and pasting code and not understanding why so please ask questions if you have them! Composition locals are a tricky topic.
zachseidner1
left a comment
zachseidner1
left a comment
There was a problem hiding this comment.
The animation makes this look so much better! There are some comments I left for you to address. Then I'll do a final pass over and we should be good.
app/src/main/java/com/cornellappdev/score/components/ErrorState.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/cornellappdev/score/components/GameDetailsLoadingState.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/cornellappdev/score/components/LoadingScreen.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/cornellappdev/score/components/LoadingScreen.kt
Outdated
Show resolved
Hide resolved
app/src/main/java/com/cornellappdev/score/components/LoadingStateBox.kt
Outdated
Show resolved
Hide resolved
zachseidner1
left a comment
zachseidner1
left a comment
There was a problem hiding this comment.
This looks great, thanks for addressing my other comments! I did a quick migration of the rest of the previews to use ScorePreview so it's obvious to future developers that they need to use this. Please merge it when you're ready 🥳
Overview
ErrorState, LoadingState, and GameDetailsLoadingState UI.
Added onRefresh function to HomeViewModel for TryAgain button functionality in ErrorState.
Changes Made
ErrorState, LoadingState, GameDetailsLoadingState to be called wherever there are ApiResponse.Loading or ApiResponse.Error.
ErrorState needs to be called within another Column with a Spacer underneath because it needs to sit on top of the bottomNavBar
LoadingState can take custom header strings, so it should be used in both HomeScreen and PastGamesScreen.
Test Coverage
Already implemented in HomeScreen, works as expected.
Next Steps (delete if not applicable)
Should the ErrorState show up after a certain amount of time has passed in the LoadingState?
Screenshots
GameDetailsLoadingState
ErrorState
LoadingState