-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
jarvislin
merged 25 commits into
feature/notifications_refresh_p2
from
issue/comment-details-content
May 24, 2024
Merged
Changes from 13 commits
Commits
Show all changes
25 commits
Select commit
Hold shift + click to select a range
8d88bfa
Update layouts and remove some redundant logic
5618cdf
Ignore the replied footer on UI
79e0915
Remove the action binding and related functions
65a6bb0
Fix lint issues
880e609
Merge branch 'feature/notifications_refresh_p2' into issue/comment-de…
348774e
Fix some UI issues related to the header
5dfac69
Add a comment
8c5d27b
Add a menu button in Comment Details
b0be301
Add the popup window
c02aace
Handle the Edit comment action
8ffe0f3
Refactor the user profile bottom sheet
2dd8b4a
Refactor the codebase related to Comment Actions
912a8d4
Reset binding to null when the view is destroyed
c660322
Handle the comment action of User Info for the comment notifications
6070f47
Display user info in the screen of My Site -> Comment List -> Comment
5456a99
Fix lint issues
2ab5cf0
Rename a variable
d1bf95b
Update a comment
a752f8a
Update layout settings
381bb1d
Fix some RTL issues
dc5cbe4
Fix a test
2b82a68
Use orEmpty() to handle the empty strings
360d311
Use get() for handling a non-null variable
d04ef21
Replace mNote with note
1b18034
Use get() to handle a non-null variable
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
659 changes: 51 additions & 608 deletions
659
WordPress/src/main/java/org/wordpress/android/ui/comments/CommentDetailFragment.java
Large diffs are not rendered by default.
Oops, something went wrong.
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
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
41 changes: 41 additions & 0 deletions
41
...ress/src/main/java/org/wordpress/android/ui/comments/unified/CommentActionPopupHandler.kt
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
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) | ||
} | ||
} |
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
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
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
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
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,8 +7,6 @@ import android.view.LayoutInflater | |
import android.view.View | ||
import android.view.ViewGroup | ||
import android.widget.FrameLayout | ||
import android.widget.ImageView | ||
import android.widget.TextView | ||
import androidx.core.view.ViewCompat | ||
import androidx.lifecycle.ViewModelProvider | ||
import androidx.lifecycle.ViewModelStoreOwner | ||
|
@@ -17,15 +15,14 @@ import com.google.android.material.bottomsheet.BottomSheetDialog | |
import com.google.android.material.bottomsheet.BottomSheetDialogFragment | ||
import org.wordpress.android.R | ||
import org.wordpress.android.WordPress | ||
import org.wordpress.android.databinding.UserProfileBottomSheetBinding | ||
import org.wordpress.android.ui.engagement.BottomSheetUiState.UserProfileUiState | ||
import org.wordpress.android.ui.utils.UiHelpers | ||
import org.wordpress.android.util.PhotonUtils | ||
import org.wordpress.android.util.PhotonUtils.Quality.HIGH | ||
import org.wordpress.android.util.UrlUtils | ||
import org.wordpress.android.util.WPAvatarUtils | ||
import org.wordpress.android.util.image.ImageManager | ||
import org.wordpress.android.util.image.ImageType.AVATAR_WITH_BACKGROUND | ||
import org.wordpress.android.util.image.ImageType.BLAVATAR | ||
import org.wordpress.android.util.image.ImageType | ||
import org.wordpress.android.viewmodel.ResourceProvider | ||
import javax.inject.Inject | ||
import com.google.android.material.R as MaterialR | ||
|
@@ -44,29 +41,44 @@ class UserProfileBottomSheetFragment : BottomSheetDialogFragment() { | |
lateinit var resourceProvider: ResourceProvider | ||
|
||
private lateinit var viewModel: UserProfileViewModel | ||
private var binding: UserProfileBottomSheetBinding? = null | ||
private val state by lazy { requireArguments().getSerializable(USER_PROFILE_STATE) as? UserProfileUiState } | ||
|
||
companion object { | ||
const val USER_PROFILE_VIEW_MODEL_KEY = "user_profile_view_model_key" | ||
|
||
fun newInstance(viewModelKey: String): UserProfileBottomSheetFragment { | ||
val fragment = UserProfileBottomSheetFragment() | ||
val bundle = Bundle() | ||
|
||
bundle.putString(USER_PROFILE_VIEW_MODEL_KEY, viewModelKey) | ||
|
||
fragment.arguments = bundle | ||
private const val USER_PROFILE_VIEW_MODEL_KEY = "user_profile_view_model_key" | ||
private const val USER_PROFILE_STATE = "user_profile_state" | ||
|
||
const val TAG = "USER_PROFILE_BOTTOM_SHEET_TAG" | ||
|
||
/** | ||
* For displaying the user profile when users are from Likes | ||
*/ | ||
fun newInstance(viewModelKey: String) = UserProfileBottomSheetFragment() | ||
.apply { | ||
arguments = Bundle().apply { | ||
putString(USER_PROFILE_VIEW_MODEL_KEY, viewModelKey) | ||
} | ||
} | ||
|
||
return fragment | ||
} | ||
/** | ||
* For displaying the user profile when users are from Comments or Notifications | ||
*/ | ||
@JvmStatic | ||
fun newInstance(state: UserProfileUiState) = UserProfileBottomSheetFragment() | ||
.apply { | ||
arguments = Bundle().apply { | ||
putSerializable(USER_PROFILE_STATE, state) | ||
} | ||
} | ||
} | ||
|
||
override fun onCreateView( | ||
inflater: LayoutInflater, | ||
container: ViewGroup?, | ||
savedInstanceState: Bundle? | ||
): View? { | ||
return inflater.inflate(R.layout.user_profile_bottom_sheet, container) | ||
} | ||
): View = UserProfileBottomSheetBinding.inflate(inflater, container, false) | ||
.apply { binding = this } | ||
.root | ||
|
||
override fun onViewCreated(view: View, savedInstanceState: Bundle?) { | ||
super.onViewCreated(view, savedInstanceState) | ||
|
@@ -77,7 +89,8 @@ class UserProfileBottomSheetFragment : BottomSheetDialogFragment() { | |
viewModel = ViewModelProvider(parentFragment as ViewModelStoreOwner, viewModelFactory) | ||
.get(vmKey, UserProfileViewModel::class.java) | ||
|
||
initObservers(view) | ||
initObservers() | ||
state?.let { binding?.setup(it) } | ||
|
||
dialog?.setOnShowListener { dialogInterface -> | ||
val sheetDialog = dialogInterface as? BottomSheetDialog | ||
|
@@ -94,68 +107,63 @@ class UserProfileBottomSheetFragment : BottomSheetDialogFragment() { | |
} | ||
} | ||
|
||
private fun initObservers(view: View) { | ||
val userAvatar = view.findViewById<ImageView>(R.id.user_avatar) | ||
val blavatar = view.findViewById<ImageView>(R.id.user_site_blavatar) | ||
val userName = view.findViewById<TextView>(R.id.user_name) | ||
val userLogin = view.findViewById<TextView>(R.id.user_login) | ||
val userBio = view.findViewById<TextView>(R.id.user_bio) | ||
val siteTitle = view.findViewById<TextView>(R.id.site_title) | ||
val siteUrl = view.findViewById<TextView>(R.id.site_url) | ||
val siteSectionHeader = view.findViewById<TextView>(R.id.site_section_header) | ||
val siteData = view.findViewById<View>(R.id.site_data) | ||
|
||
viewModel.bottomSheetUiState.observe(viewLifecycleOwner, { state -> | ||
private fun initObservers() { | ||
viewModel.bottomSheetUiState.observe(viewLifecycleOwner) { state -> | ||
when (state) { | ||
is UserProfileUiState -> { | ||
val avatarSz = resourceProvider.getDimensionPixelSize(R.dimen.user_profile_bottom_sheet_avatar_sz) | ||
val blavatarSz = resourceProvider.getDimensionPixelSize(R.dimen.avatar_sz_medium) | ||
|
||
imageManager.loadIntoCircle( | ||
userAvatar, | ||
AVATAR_WITH_BACKGROUND, | ||
WPAvatarUtils.rewriteAvatarUrl(state.userAvatarUrl, avatarSz) | ||
) | ||
userName.text = state.userName | ||
userLogin.text = if (state.userLogin.isNotBlank()) { | ||
getString(R.string.at_username, state.userLogin) | ||
} else { | ||
"" | ||
} | ||
if (state.userBio.isNotBlank()) { | ||
userBio.text = state.userBio | ||
userBio.visibility = View.VISIBLE | ||
} else { | ||
userBio.visibility = View.GONE | ||
} | ||
|
||
imageManager.load( | ||
blavatar, | ||
BLAVATAR, | ||
PhotonUtils.getPhotonImageUrl(state.blavatarUrl, blavatarSz, blavatarSz, HIGH) | ||
) | ||
|
||
if (state.hasSiteUrl) { | ||
siteTitle.text = state.siteTitle | ||
siteUrl.text = UrlUtils.getHost(state.siteUrl) | ||
siteData.setOnClickListener { | ||
state.onSiteClickListener?.invoke( | ||
state.siteId, | ||
state.siteUrl, | ||
state.blogPreviewSource | ||
) | ||
} | ||
siteSectionHeader.visibility = View.VISIBLE | ||
blavatar.visibility = View.VISIBLE | ||
siteData.visibility = View.VISIBLE | ||
} else { | ||
siteSectionHeader.visibility = View.GONE | ||
blavatar.visibility = View.GONE | ||
siteData.visibility = View.GONE | ||
} | ||
binding?.setup(state) | ||
} | ||
} | ||
}) | ||
} | ||
} | ||
|
||
private fun UserProfileBottomSheetBinding.setup(state: UserProfileUiState) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I only moved the code snippets to this function, the logic is not changed. |
||
val avatarSz = | ||
resourceProvider.getDimensionPixelSize(R.dimen.user_profile_bottom_sheet_avatar_sz) | ||
val blavatarSz = resourceProvider.getDimensionPixelSize(R.dimen.avatar_sz_medium) | ||
|
||
imageManager.loadIntoCircle( | ||
userAvatar, | ||
ImageType.AVATAR_WITH_BACKGROUND, | ||
WPAvatarUtils.rewriteAvatarUrl(state.userAvatarUrl, avatarSz) | ||
) | ||
userName.text = state.userName | ||
userLogin.text = if (state.userLogin.isNotBlank()) { | ||
getString(R.string.at_username, state.userLogin) | ||
} else { | ||
"" | ||
} | ||
if (state.userBio.isNotBlank()) { | ||
userBio.text = state.userBio | ||
userBio.visibility = View.VISIBLE | ||
} else { | ||
userBio.visibility = View.GONE | ||
} | ||
|
||
imageManager.load( | ||
userSiteBlavatar, | ||
ImageType.BLAVATAR, | ||
PhotonUtils.getPhotonImageUrl(state.blavatarUrl, blavatarSz, blavatarSz, PhotonUtils.Quality.HIGH) | ||
) | ||
|
||
if (state.hasSiteUrl) { | ||
siteTitle.text = state.siteTitle | ||
siteUrl.text = UrlUtils.getHost(state.siteUrl) | ||
siteData.setOnClickListener { | ||
state.onSiteClickListener?.invoke( | ||
state.siteId, | ||
state.siteUrl, | ||
state.blogPreviewSource | ||
) | ||
} | ||
siteSectionHeader.visibility = View.VISIBLE | ||
userSiteBlavatar.visibility = View.VISIBLE | ||
siteData.visibility = View.VISIBLE | ||
} else { | ||
siteSectionHeader.visibility = View.GONE | ||
userSiteBlavatar.visibility = View.GONE | ||
siteData.visibility = View.GONE | ||
} | ||
} | ||
|
||
override fun onAttach(context: Context) { | ||
|
@@ -167,4 +175,9 @@ class UserProfileBottomSheetFragment : BottomSheetDialogFragment() { | |
super.onCancel(dialog) | ||
viewModel.onBottomSheetCancelled() | ||
} | ||
|
||
override fun onDestroyView() { | ||
super.onDestroyView() | ||
binding = null | ||
} | ||
} |
Oops, something went wrong.
Oops, something went wrong.
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.
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'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 foronSiteClickListener
.)Just leaving this note for your consideration :)
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 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.