Skip to content
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-1968:UX – Add comment composer to thread #1291

Merged
merged 12 commits into from
Jun 17, 2021

Conversation

hadia
Copy link
Contributor

@hadia hadia commented Jun 15, 2021

πŸ“² What

Add comment composer to thread Activity

πŸ€” Why

image

πŸ›  How

1- pass project from comment activity to thread activity
2- add the same logic to check when to show comment composer

πŸ‘€ See

device-2021-06-15-163912.mp4
device-2021-06-15-161330.mp4

πŸ“‹ QA

Same As Comment Composer
Make sure you have a project that you are not backing, a project that you are backing, and a project that you created, and all projects must have comments. Test going to the comments directly from the project page, as well as from the updates page. Then test the different states listed below.

πŸ”΄ = no comment composer
🟑 = comment composer disabled
🟒 = comment composer enabled

These are the different states to test:
Logged out = πŸ”΄
Logged in and not backing = 🟑
Logged in and backing = 🟒
Logged in and creator = 🟒

-QA
Showing keyboard at the start when pressing replay
and keyboard is hidden in case pressing view replies

Story πŸ“–

https://kickstarter.atlassian.net/browse/NT-1968

@@ -217,9 +225,10 @@ class CommentsActivity :
* // TODO: Once the viewReplies UI is completed call this method with openKeyboard = false
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can remove now the TODO's :)

@@ -193,7 +201,7 @@ class CommentsActivity :
}

override fun onReplyButtonClicked(comment: Comment) {
startThreadActivity(comment, true)
viewModel.inputs.onReplayClicked(comment, true)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: "replay" should be "reply"

@@ -204,7 +212,7 @@ class CommentsActivity :
}

override fun onCommentRepliesClicked(comment: Comment) {
startThreadActivity(comment, false)
viewModel.inputs.onReplayClicked(comment, false)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: same as comment above.

}

private fun getCommentComposerStatus(projectAndUser: Pair<Project, User?>) =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if we can abstract this logic into something separate since this is being reused in a couple places? Just a thought.

Copy link
Contributor

@leighdouglas leighdouglas left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Just a small thing, I made a comment about the spelling of replay that should be corrected to reply -> I noticed that most instances of this word are spelled incorrectly. Could you take a sec to look through and make sure they're all spelled correctly? Otherwise great job! πŸŽ‰ πŸš€

Copy link
Contributor

@Arkariang Arkariang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm! as soon as the linter is fixed is good to go πŸ‘

@codecov
Copy link

codecov bot commented Jun 15, 2021

Codecov Report

Merging #1291 (e8d8f60) into master (4bb43e3) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##             master    #1291   +/-   ##
=========================================
  Coverage     74.34%   74.34%           
  Complexity      739      739           
=========================================
  Files           221      221           
  Lines          6646     6646           
  Branches        405      405           
=========================================
  Hits           4941     4941           
  Misses         1569     1569           
  Partials        136      136           

Continue to review full report at Codecov.

Legend - Click here to learn more
Ξ” = absolute <relative> (impact), ΓΈ = not affected, ? = missing data
Powered by Codecov. Last update 4bb43e3...e8d8f60. Read the comment docs.

@@ -68,6 +70,7 @@ interface CommentsViewModel {
private val refresh = PublishSubject.create<Void>()
private val nextPage = PublishSubject.create<Void>()
private val onShowGuideLinesLinkClicked = PublishSubject.create<Void>()
private val onReplayClicked = PublishSubject.create<Pair<Comment, Boolean>>()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be corrected to onReplyClicked

@@ -342,6 +353,9 @@ interface CommentsViewModel {
override fun isRefreshing(): Observable<Boolean> = isRefreshing
override fun insertNewCommentToList(comment: String, createdAt: DateTime) = insertNewCommentToList.onNext(Pair(comment, createdAt))

override fun onReplyClicked(comment: Comment, openKeyboard: Boolean) = onReplayClicked.onNext(Pair(comment, openKeyboard))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

likewise, this should be onReplyClicked

@@ -22,6 +28,10 @@ interface ThreadViewModel {

/** Will tell to the compose view if should open the keyboard */
fun shouldFocusOnCompose(): Observable<Boolean>

fun currentUserAvatar(): Observable<String?>
fun replayComposerStatus(): Observable<CommentComposerStatus>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: replyComposerStatus()

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

@@ -30,6 +40,9 @@ interface ThreadViewModel {

private val rootComment = BehaviorSubject.create<Comment>()
private val focusOnCompose = BehaviorSubject.create<Boolean>()
private val currentUserAvatar = BehaviorSubject.create<String?>()
private val replayComposerStatus = BehaviorSubject.create<CommentComposerStatus>()
private val showReplayComposer = BehaviorSubject.create<Boolean>()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: change to replyComposerStatus and showReplyComposer

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

private fun getCommentFromIntent() = intent()
.map { it.getParcelableExtra(IntentKey.COMMENT) as Comment? }
.ofType(Comment::class.java)

override fun getRootComment(): Observable<Comment> = this.rootComment
override fun shouldFocusOnCompose(): Observable<Boolean> = this.focusOnCompose
override fun currentUserAvatar(): Observable<String?> = currentUserAvatar
override fun replayComposerStatus(): Observable<CommentComposerStatus> = replayComposerStatus
override fun showReplayComposer(): Observable<Boolean> = showReplayComposer
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: change to replyComposerStatus() and showReplyComposer()

@@ -16,6 +20,9 @@ class ThreadViewModelTest : KSRobolectricTestCase() {
private val getComment = TestSubscriber<Comment>()
private val focusCompose = TestSubscriber<Boolean>()

private val replayComposerStatus = TestSubscriber<CommentComposerStatus>()
private val showReplayComposer = TestSubscriber<Boolean>()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: change to replyComposerStatus and showReplyComposer

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

# Conflicts:
#	app/src/main/java/com/kickstarter/viewmodels/CommentsViewModel.kt
# Conflicts:
#	app/src/test/java/com/kickstarter/viewmodels/CommentsViewModelTest.kt
@hadia hadia merged commit 1bf8878 into master Jun 17, 2021
@hadia hadia deleted the hadia/Add_comment_composer_to_thread branch June 17, 2021 19:52
Arkariang added a commit that referenced this pull request Jun 21, 2021
…tin/NT-2059-update-stripe-sdk

* 'master' of github.com:kickstarter/android-oss:
  - previous version build code
  NT-1968:UX – Add comment composer to thread (#1291)
  Remove underline (#1296)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants