Conversation
There was a problem hiding this comment.
Pull request overview
This PR adjusts bottom spacing and collapsed peek-height behavior for user/group bottom sheets, along with related UI padding tweaks, to improve layout near the bottom edge of the screen.
Changes:
- Tweaks layout attributes for the user bottom sheet’s expandable text view.
- Adjusts padding/margins in the member upgrade payment button composable.
- Refactors User/Group bottom sheet peek-height computation and bottom spacing (including safe-area handling changes).
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 7 comments.
| File | Description |
|---|---|
| app/src/main/res/layout/fragment_user_bottom_sheet.xml | Minor attribute reordering/formatting for the expandable detail text view. |
| app/src/main/java/one/mixin/android/ui/setting/ui/components/MemberUpgradePaymentButton.kt | Reduces vertical padding and normalizes button padding to horizontal-only. |
| app/src/main/java/one/mixin/android/ui/common/UserBottomSheetDialogFragment.kt | Adds fixed bottom margin for ops, introduces dynamic collapsed peek-height calculation and update flow, changes safe-bottom handling for menu layout. |
| app/src/main/java/one/mixin/android/ui/common/GroupBottomSheetDialogFragment.kt | Mirrors User bottom sheet changes: fixed bottom margin, dynamic collapsed peek-height calculation/update, safe-bottom handling adjustment. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| private fun updateCollapsedPeekHeight() { | ||
| val peekHeight = calculateCollapsedPeekHeight() | ||
| if (peekHeight == 0) { | ||
| menuListLayout?.doOnPreDraw { updateCollapsedPeekHeight() } | ||
| return | ||
| } | ||
|
|
||
| if (behavior?.peekHeight != peekHeight) { | ||
| behavior?.peekHeight = peekHeight | ||
| } | ||
| } |
There was a problem hiding this comment.
updateCollapsedPeekHeight() sets behavior.peekHeight directly from calculateCollapsedPeekHeight(), bypassing Context.resolveBottomSheetPeekHeight(...) used by MixinScrollableBottomSheetDialogFragment. This can drop safe-area bottom inset handling and make the updated peekHeight inconsistent with the initial peek height. Consider resolving the computed height via resolveBottomSheetPeekHeight(contentView, computedHeight) (or otherwise incorporating getSafeAreaInsetsBottom consistently) before assigning peekHeight.
| return titleHeight + scrollContentHeight - | ||
| menuHeight - if (menuListLayout != null) 42.dp else 12.dp + requireContext().navigationBarHeight() + 16.dp | ||
| } |
There was a problem hiding this comment.
calculateCollapsedPeekHeight() adds navigationBarHeight() to the returned value, but the base class already applies resolveBottomSheetPeekHeight(...), which adds safe-area bottom insets when needed. This risks double-counting bottom insets (and can cause a visible jump between initial peekHeight and later updates). Prefer returning a content-only height here and letting resolveBottomSheetPeekHeight handle system insets consistently.
| val circleNames = bottomViewModel.findCirclesNameByConversationId(conversationId) | ||
| val conversation = bottomViewModel.getConversation(conversationId) | ||
| initMenu(u, circleNames, conversation) | ||
|
|
There was a problem hiding this comment.
There is trailing whitespace on this blank line. This often trips formatting/ktlint checks; please remove the extra spaces.
| private fun View.heightWithMargins(width: Int): Int { | ||
| val params = layoutParams as? ViewGroup.MarginLayoutParams | ||
| return measuredHeightForWidth(width) + (params?.topMargin ?: 0) + (params?.bottomMargin ?: 0) | ||
| } | ||
|
|
||
| private fun View.measuredHeightForWidth(width: Int): Int { | ||
| if (measuredHeight > 0) { | ||
| return measuredHeight | ||
| } | ||
| measure( | ||
| View.MeasureSpec.makeMeasureSpec(width, View.MeasureSpec.EXACTLY), | ||
| View.MeasureSpec.makeMeasureSpec(0, View.MeasureSpec.UNSPECIFIED), | ||
| ) | ||
| return measuredHeight | ||
| } |
There was a problem hiding this comment.
The helper methods measuredHeightForWidth()/heightWithMargins() are introduced here and duplicated in GroupBottomSheetDialogFragment. Consider extracting them into a shared extension (or the base bottom sheet class) to avoid divergence and keep peek-height logic consistent across bottom sheets.
| lifecycleScope.launch { | ||
| val circleNames = bottomViewModel.findCirclesNameByConversationId(conversationId) | ||
| initMenu(localMe, circleNames) | ||
|
|
There was a problem hiding this comment.
There is trailing whitespace on this blank line. This often trips formatting/ktlint checks; please remove the extra spaces.
| private fun updateCollapsedPeekHeight() { | ||
| val peekHeight = calculateCollapsedPeekHeight() | ||
| if (peekHeight == 0) { | ||
| menuListLayout?.doOnPreDraw { updateCollapsedPeekHeight() } | ||
| return | ||
| } | ||
|
|
||
| if (behavior?.peekHeight != peekHeight) { | ||
| behavior?.peekHeight = peekHeight | ||
| } | ||
| } |
There was a problem hiding this comment.
updateCollapsedPeekHeight() assigns behavior.peekHeight directly from calculateCollapsedPeekHeight(), but MixinScrollableBottomSheetDialogFragment sets peekHeight via resolveBottomSheetPeekHeight(...) (which handles safe-area insets). This can cause the updated peekHeight to ignore bottom insets (especially on gesture nav devices) and differ from the initial resolved peek height. Consider resolving the computed height via resolveBottomSheetPeekHeight(contentView, computedHeight) before setting peekHeight.
| private fun View.heightWithMargins(width: Int): Int { | ||
| val params = layoutParams as? ViewGroup.MarginLayoutParams | ||
| return measuredHeightForWidth(width) + (params?.topMargin ?: 0) + (params?.bottomMargin ?: 0) | ||
| } | ||
|
|
||
| private fun View.measuredHeightForWidth(width: Int): Int { | ||
| if (measuredHeight > 0) { | ||
| return measuredHeight | ||
| } | ||
| measure( | ||
| View.MeasureSpec.makeMeasureSpec(width, View.MeasureSpec.EXACTLY), | ||
| View.MeasureSpec.makeMeasureSpec(0, View.MeasureSpec.UNSPECIFIED), | ||
| ) | ||
| return measuredHeight | ||
| } |
There was a problem hiding this comment.
The helper methods measuredHeightForWidth()/heightWithMargins() are duplicated between GroupBottomSheetDialogFragment and UserBottomSheetDialogFragment. Extracting them into a shared extension/base utility would reduce the risk of the two peek-height implementations drifting over time.
SeniorZhai
force-pushed
the
fix/bottomsheet-edge-to-edge
branch
from
c35d558 to
4561b5d
Compare
|
@claude[agent] review |
No description provided.