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
NT-1907:UI for Comment Card #1255
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1255 +/- ##
=========================================
Coverage 74.50% 74.50%
Complexity 739 739
=========================================
Files 221 221
Lines 6632 6632
Branches 404 404
=========================================
Hits 4941 4941
Misses 1555 1555
Partials 136 136 Continue to review full report at Codecov.
|
|
||
fun takeData(comments: List<Comment>) { | ||
sections().clear() | ||
addSection(Observable.from(comments).toList().toBlocking().single()) |
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.
Not fan of blocking observables,
usually on the adapter for creating a section we do that on the init Block, take a look here
android-oss/app/src/main/java/com/kickstarter/ui/adapters/ActivityFeedAdapter.kt
Line 161 in a38f215
insertSection(SECTION_LOGGED_IN_EMPTY_VIEW, emptyList<Any>()) |
or here
insertSection(SECTION_EMPTY_VIEW, emptyList<Any>()) |
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.
|
||
fun setAvatarUrl(url: String?) { | ||
url?.let { | ||
Picasso.get().load(it) |
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 is a picky comment for the future, I would like to not speak directly to a dependency, this comment is more meant for future tech debt as I know this concrete case with picasso is quite extended throwout the app so no need to act on it.
But we should create or a utils class or a client to inject on the UI layer that will isolate us from calling any third party directly
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 also create an extension layer or Custom Imageview using through app
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.
totally agree
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.
import com.kickstarter.ui.extensions.parseHtmlTag | ||
import com.squareup.picasso.Picasso | ||
|
||
class CommentCard @JvmOverloads constructor( |
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.
@@ -25,18 +28,22 @@ interface CommentsViewModel { | |||
fun currentUserAvatar(): Observable<String?> | |||
fun enableCommentComposer(): Observable<Boolean> | |||
fun showCommentComposer(): Observable<Void> | |||
fun commentsList(): Observable<List<Comment>?> |
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.
Does it needs to be optional? in case of no comments we could have an empty list so no null allowed on this Observable
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.
Changed
android:focusable="true" | ||
android:orientation="vertical"> | ||
|
||
<com.kickstarter.ui.views.CommentCard |
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.
import org.junit.Before | ||
import org.junit.Test | ||
|
||
class CommentCardTest : KSRobolectricTestCase() { |
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.
AWESOME!!! 💞
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.
I left a minor comment on the CommentsAdapter
. Apart from that you've done an exceptional work here @hadia , Love it!
… and it will be handle in different ticket
@Arkariang @leighdouglas @sunday-okpoluaefe @jgsamudio Remove part of handling empty state as app crash in case logout user and it will be handle in different ticket |
📲 What
A reusable component has been created to render a single comment
🤔 Why
Build new comment UI
🛠 How
👀 See
device-2021-05-21-203700.mp4
📋 QA
open comment list from any project
Story 📖
https://kickstarter.atlassian.net/browse/NT-1907