[Placeholder] Prototype for new library #457
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Thanks, I'll take a look at this tomorrow once the next release is out. |
chrisbanes
reviewed
Jun 3, 2021
placeholder/src/main/java/com/google/accompanist/placeholder/Placeholder.kt
Outdated
Show resolved
Hide resolved
placeholder/src/main/java/com/google/accompanist/placeholder/Placeholder.kt
Outdated
Show resolved
Hide resolved
placeholder/src/main/java/com/google/accompanist/placeholder/Placeholder.kt
Outdated
Show resolved
Hide resolved
| return result | ||
| } | ||
|
|
||
| override fun equals(other: Any?): Boolean { |
There was a problem hiding this comment.
No need for equals/hashcode here.
| import androidx.compose.ui.graphics.lerp | ||
|
|
||
| internal object PlaceholderDefaults { | ||
| val PlaceholderColor = Color.Gray.copy(alpha = 0.5f) |
There was a problem hiding this comment.
Did you use Gray here to avoid a dependency on Material? Ideally this would be set to contentColor() but I'm not sure that we want to depend on Material
There was a problem hiding this comment.
Yes, I used Gray to avoid dependency on Material.
But if it's better to rely on the material, I'll change.
placeholder/src/main/java/com/google/accompanist/placeholder/PlaceholderAnimatedBrush.kt
Outdated
Show resolved
Hide resolved
placeholder/src/main/java/com/google/accompanist/placeholder/PlaceholderAnimatedBrush.kt
Show resolved
Hide resolved
placeholder/src/main/java/com/google/accompanist/placeholder/PlaceholderAnimatedBrush.kt
Outdated
Show resolved
Hide resolved
placeholder/src/main/java/com/google/accompanist/placeholder/PlaceholderAnimatedBrush.kt
Outdated
Show resolved
Hide resolved
placeholder/src/main/java/com/google/accompanist/placeholder/PlaceholderAnimatedBrush.kt
Outdated
Show resolved
Hide resolved
- Add the explicit return type. - Only create one modifier on every animation frame. - Remove equals/hashcode in Placeholder. - Use default easing instead of LinearEasing. - Change PlaceholderDefaults to public class. - Move functions for PlaceholderAnimatedBrush inside the companion object. - Add kdoc.
|
I applied all comments. 🙇 |
chrisbanes
approved these changes
Jun 3, 2021
|
Omg 😍 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
I created a prototype for content placeholders.
If it's okay to go ahead, I'll add some test code.
Or, if you advise on the direction, I will correct it.
New APIs:
Modifier.placeholder(visible: Boolean)to show a content placeholder.using
PlaceholderAnimatedBrushlikefadeBrush()orshimmerBrush().Fixes #336
Demo