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

The redesign of comment details content #20798

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view

Large diffs are not rendered by default.

Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,12 @@ import android.os.Bundle
import androidx.core.view.isGone
import org.wordpress.android.R
import org.wordpress.android.datasets.NotificationsTable
import org.wordpress.android.fluxc.tools.FormattableRangeType
import org.wordpress.android.models.Note
import org.wordpress.android.ui.comments.unified.CommentIdentifier
import org.wordpress.android.ui.comments.unified.CommentSource
import org.wordpress.android.ui.engagement.BottomSheetUiState
import org.wordpress.android.ui.reader.tracker.ReaderTracker.Companion.SOURCE_NOTIF_COMMENT_USER_PROFILE
import org.wordpress.android.util.AppLog
import org.wordpress.android.util.ToastUtils

Expand All @@ -16,6 +21,9 @@ import org.wordpress.android.util.ToastUtils
* It'd be better to have multiple fragments for different sources for different purposes
*/
class NotificationCommentDetailFragment : CommentDetailFragment() {
private val note: Note // note will be non-null after onCreate
get() = mNote!!
Copy link
Contributor

Choose a reason for hiding this comment

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

Wdyt of using this getter for all the mNote!! occurrences in this file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, that makes sense, I missed some occurrences in this file when I made the change.


override fun onCreate(savedInstanceState: Bundle?) {
super.onCreate(savedInstanceState)

Expand All @@ -26,6 +34,26 @@ class NotificationCommentDetailFragment : CommentDetailFragment() {
}
}

override fun getUserProfileUiState(): BottomSheetUiState.UserProfileUiState {
val user = mContentMapper.mapToFormattableContentList(note.body.toString())
.find { FormattableRangeType.fromString(it.type) == FormattableRangeType.USER }

return BottomSheetUiState.UserProfileUiState(
userAvatarUrl = note.iconURL,
blavatarUrl = "",
userName = user?.text ?: getString(R.string.anonymous),
userLogin = mComment?.authorEmail.orEmpty(),
userBio = "",
siteTitle = user?.meta?.titles?.home ?: getString(R.string.user_profile_untitled_site),
siteUrl = user?.ranges?.firstOrNull()?.url.orEmpty(),
siteId = user?.meta?.ids?.site ?: 0L,
blogPreviewSource = SOURCE_NOTIF_COMMENT_USER_PROFILE
)
}

override fun getCommentIdentifier(): CommentIdentifier =
CommentIdentifier.NotificationCommentIdentifier(note.id, note.commentId);

override fun handleHeaderVisibility() {
mBinding?.headerView?.isGone = true
}
Expand All @@ -42,10 +70,10 @@ class NotificationCommentDetailFragment : CommentDetailFragment() {
mSite = mSiteStore.getSiteBySiteId(note.siteId.toLong())
if (mSite == null) {
// This should not exist, we should clean that screen so a note without a site/comment can be displayed
mSite = createDummyWordPressComSite(mNote!!.siteId.toLong())
mSite = createDummyWordPressComSite(note.siteId.toLong())
}
if (mBinding != null && mReplyBinding != null && mActionBinding != null) {
showComment(mBinding!!, mReplyBinding!!, mActionBinding!!, mSite!!, mComment, mNote)
if (mBinding != null && mReplyBinding != null) {
showComment(mBinding!!, mReplyBinding!!, mSite!!, mComment, note)
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,16 +4,27 @@ package org.wordpress.android.ui.comments

import android.os.Bundle
import androidx.core.view.isVisible
import com.gravatar.AvatarQueryOptions
import com.gravatar.AvatarUrl
import com.gravatar.types.Email
import org.wordpress.android.R
import org.wordpress.android.fluxc.model.CommentModel
import org.wordpress.android.fluxc.model.SiteModel
import org.wordpress.android.ui.comments.unified.CommentIdentifier
import org.wordpress.android.ui.comments.unified.CommentSource
import org.wordpress.android.ui.engagement.BottomSheetUiState
import org.wordpress.android.ui.reader.tracker.ReaderTracker
import org.wordpress.android.util.WPAvatarUtils

/**
* Used when called from comment list
* [CommentDetailFragment] is too big to be reused
* It'd be better to have multiple fragments for different sources for different purposes
*/
class SiteCommentDetailFragment : CommentDetailFragment() {
private val comment: CommentModel
get() = mComment!!

override fun onCreate(savedInstanceState: Bundle?) {
super.onCreate(savedInstanceState)
if (savedInstanceState != null) {
Expand All @@ -23,6 +34,28 @@ class SiteCommentDetailFragment : CommentDetailFragment() {
}
}

override fun getUserProfileUiState(): BottomSheetUiState.UserProfileUiState {
return BottomSheetUiState.UserProfileUiState(
userAvatarUrl = CommentExtension.getAvatarUrl(
comment,
resources.getDimensionPixelSize(R.dimen.avatar_sz_large)
),
blavatarUrl = "",
userName = comment.authorName ?: getString(R.string.anonymous),
userLogin = comment.authorEmail.orEmpty(),
// keep them empty because there's no data for displaying on UI
userBio = "",
siteTitle = "",
siteUrl = "",
siteId = 0L,
blogPreviewSource = ReaderTracker.SOURCE_SITE_COMMENTS_USER_PROFILE
)
}

override fun getCommentIdentifier(): CommentIdentifier =
CommentIdentifier.SiteCommentIdentifier(comment.id, comment.remoteCommentId)


override fun handleHeaderVisibility() {
mBinding?.headerView?.isVisible = true
}
Expand All @@ -48,3 +81,19 @@ class SiteCommentDetailFragment : CommentDetailFragment() {
}
}
}

object CommentExtension {
fun getAvatarUrl(comment: CommentModel, size: Int): String = when {
comment.authorProfileImageUrl != null -> WPAvatarUtils.rewriteAvatarUrl(
comment.authorProfileImageUrl!!,
size
)

comment.authorEmail != null -> AvatarUrl(
Email(comment.authorEmail!!),
AvatarQueryOptions(size, null, null, null)
).url().toString()

else -> ""
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
@file:Suppress("DEPRECATION")

package org.wordpress.android.ui.comments.unified

import android.view.LayoutInflater
import android.view.View
import android.widget.PopupWindow
import org.wordpress.android.R
import org.wordpress.android.databinding.CommentActionsBinding
import org.wordpress.android.ui.comments.CommentDetailFragment
import org.wordpress.android.util.ToastUtils

object CommentActionPopupHandler {
@JvmStatic
fun show(anchorView: View, listener: CommentDetailFragment.OnActionClickListener?) {
val popupWindow = PopupWindow(anchorView.context, null, R.style.WordPress)
popupWindow.isOutsideTouchable = true
popupWindow.elevation = anchorView.context.resources.getDimension(R.dimen.popup_over_toolbar_elevation)
popupWindow.contentView = CommentActionsBinding
.inflate(LayoutInflater.from(anchorView.context))
.apply {
textUserInfo.setOnClickListener {
listener?.onUserInfoClicked()
popupWindow.dismiss()
}
textShare.setOnClickListener {
ToastUtils.showToast(it.context, "not yet implemented")
popupWindow.dismiss()
}
textEditComment.setOnClickListener {
listener?.onEditCommentClicked()
popupWindow.dismiss()
}
textChangeStatus.setOnClickListener {
ToastUtils.showToast(it.context, "not yet implemented")
popupWindow.dismiss()
}
}.root
popupWindow.showAsDropDown(anchorView)
}
}
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
package org.wordpress.android.ui.engagement

import java.io.Serializable

sealed class BottomSheetUiState {
@Suppress("SerialVersionUIDInSerializableClass")
data class UserProfileUiState(
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm wondering if we shouldn't mark some fields as nullable because not all are always available for all the cases. For example, if the user does not have an avatar URL, it should be null but not an empty string that has less straight meaning. When using the state in different places with not enough context, it looks like the state has to contain all the data (except for onSiteClickListener.)
Just leaving this note for your consideration :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I 100% agree with you, straight meaning is important for us to handle those fields correctly. I find many implementations use default values, but it'd be better if they are defined as nullable.

I suspect that when handling this data with Java in the past, there was a tendency to use defensive programming to avoid crashes. However, now with Kotlin, handling nullables has become much easier. I think we can find time to refactor this.

val userAvatarUrl: String,
val blavatarUrl: String,
Expand All @@ -10,9 +13,8 @@ sealed class BottomSheetUiState {
val siteTitle: String,
val siteUrl: String,
val siteId: Long,
val onSiteClickListener: ((siteId: Long, siteUrl: String, source: String) -> Unit)? = null,
val blogPreviewSource: String
) : BottomSheetUiState() {
) : BottomSheetUiState(), Serializable {
val hasSiteUrl: Boolean = siteUrl.isNotBlank()
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -111,14 +111,14 @@ class EngagedPeopleListFragment : Fragment() {
recycler.layoutManager = layoutManager

userProfileViewModel.onBottomSheetAction.observeEvent(viewLifecycleOwner) { state ->
var bottomSheet = childFragmentManager.findFragmentByTag(USER_PROFILE_BOTTOM_SHEET_TAG)
var bottomSheet = childFragmentManager.findFragmentByTag(UserProfileBottomSheetFragment.TAG)
as? UserProfileBottomSheetFragment

when (state) {
ShowBottomSheet -> {
if (bottomSheet == null) {
bottomSheet = UserProfileBottomSheetFragment.newInstance(USER_PROFILE_VM_KEY)
bottomSheet.show(childFragmentManager, USER_PROFILE_BOTTOM_SHEET_TAG)
bottomSheet.show(childFragmentManager, UserProfileBottomSheetFragment.TAG)
}
}

Expand Down Expand Up @@ -192,7 +192,7 @@ class EngagedPeopleListFragment : Fragment() {
}

is OpenUserProfileBottomSheet -> {
userProfileViewModel.onBottomSheetOpen(event.userProfile, event.onClick, event.source)
userProfileViewModel.onBottomSheetOpen(event.userProfile, event.source)
}
}

Expand Down Expand Up @@ -304,8 +304,6 @@ class EngagedPeopleListFragment : Fragment() {
private const val KEY_LIST_SCENARIO = "list_scenario"
private const val KEY_LIST_STATE = "list_state"

private const val USER_PROFILE_BOTTOM_SHEET_TAG = "USER_PROFILE_BOTTOM_SHEET_TAG"

@JvmStatic
fun newInstance(listScenario: ListScenario): EngagedPeopleListFragment {
val args = Bundle()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,7 @@ class ListScenarioUtils @Inject constructor(
imageManager,
notificationsUtilsWrapper
)
headerNoteBlock.setIsComment(note.isCommentType)
headerNoteBlock.setReplyToComment(note.isCommentReplyType)

val spannable: Spannable = notificationsUtilsWrapper.getSpannableContentForRanges(headerNoteBlock.getHeader(0))
val spans = spannable.getSpans(
Expand Down
Loading
Loading