Skip to content

Conversation

@nbradbury
Copy link
Contributor

@nbradbury nbradbury commented Dec 4, 2025

Currently HENewTicketScreen and HEConversationDetailScreen are cut off. This PR resolves this.

To test

  • Use an API 35 device with three button navigation enabled
  • Verify the content of the above two screens is no longer cut off by the navigation bar
  • Use a pre-API 35 device and verify the two screens still look fine

Before

before

After

after

@dangermattic
Copy link
Collaborator

dangermattic commented Dec 4, 2025

1 Warning
⚠️ PR is not assigned to a milestone.

Generated by 🚫 Danger

@nbradbury
Copy link
Contributor Author

@claude

@claude
Copy link
Contributor

claude bot commented Dec 4, 2025

Claude finished @nbradbury's task —— View job


Code Review Progress

  • Read and analyze changed files
  • Review code quality and best practices
  • Check for security considerations
  • Evaluate performance implications
  • Assess testing needs
  • Review documentation

Code Review Summary

I've reviewed the PR changes for edge-to-edge support in the HE support screens. Overall, the implementation is solid and correctly addresses the edge-to-edge cutoff issue. Here's my detailed feedback:

✅ Strengths

  1. Correct Edge-to-Edge Implementation: The addition of navigationBarsPadding() to both bottom bars properly handles system navigation bars in edge-to-edge mode.

  2. Consistent Pattern: Both screens apply the same fix consistently - wrapping the bottom bar content in a Box with navigationBarsPadding().

  3. No Breaking Changes: The changes are minimal and focused, without affecting existing functionality.

  4. Well-Structured Code: The Compose code is clean, readable, and follows Material 3 design patterns.

🔍 Observations

HEConversationDetailScreen.kt (lines 127-142)

The fix wraps both the ClosedConversationBanner() and ReplyButton() in a Box with navigationBarsPadding():

Box(modifier = Modifier.navigationBarsPadding()) {
    if (isClosed) {
        ClosedConversationBanner()
    } else {
        ReplyButton(...)
    }
}

This is the right approach for edge-to-edge support.

HENewTicketScreen.kt (lines 97-139)

The bottom bar was refactored to inline the SendButton composable and add proper edge-to-edge padding:

  • Added navigationBarsPadding() to the outer Surface
  • Kept imePadding() on the inner Box for keyboard handling
  • This layered approach correctly handles both navigation bars and keyboard

💡 Suggestions

1. Code Consistency - Duplicate SendButton Composable

The SendButton composable (lines 269-309) appears to be unused now since its logic was inlined into the bottomBar. Consider removing it to avoid confusion:

@Composable
private fun SendButton(
    enabled: Boolean,
    isLoading: Boolean,
    onClick: () -> Unit
) { ... }

Action: Remove the unused SendButton composable or add a comment explaining why it's kept.

2. Minor Inconsistency in Button Styling

In HENewTicketScreen.kt, the inlined button uses:

  • size(20.dp) for the progress indicator
  • No explicit strokeWidth set

While the removed SendButton composable had:

  • size(24.dp) for the progress indicator
  • strokeWidth = 2.5.dp

The current implementation (20.dp) is actually better for the 52.dp button height, but this was an unintentional change. Consider if this was intended.

3. Accessibility Consideration

Both implementations look good for accessibility. The content descriptions and semantics are properly maintained.

🔒 Security

No security concerns identified. The changes are purely UI-related and don't introduce any vulnerabilities.

⚡ Performance

No performance concerns. The changes are minimal and don't introduce any expensive operations or unnecessary recompositions.

🧪 Testing Recommendations

  1. Manual Testing Checklist:

    • ✅ Test on devices with gesture navigation (edge-to-edge)
    • ✅ Test on devices with button navigation
    • ✅ Test with keyboard open/closed in HENewTicketScreen
    • ✅ Test in both light and dark themes
    • ✅ Test the closed conversation banner state
    • ✅ Verify button click areas are not overlapped
  2. Suggested Test Cases:

    • Verify bottom content is not cut off on various screen sizes
    • Test with different system UI modes (immersive, standard)
    • Verify smooth keyboard transitions don't cause jumping

📝 Documentation

No documentation updates needed. The changes are self-explanatory and follow standard Compose patterns.

🎯 Verdict

Approved - The PR effectively fixes the edge-to-edge cutoff issue with minimal, focused changes. The only suggestion is to clean up the now-unused SendButton composable in HENewTicketScreen.kt.


@wpmobilebot
Copy link
Contributor

wpmobilebot commented Dec 4, 2025

App Icon📲 You can test the changes from this Pull Request in WordPress by scanning the QR code below to install the corresponding build.
App NameWordPress
FlavorJalapeno
Build TypeDebug
Versionpr22397-33fd5da
Commit33fd5da
Direct Downloadwordpress-prototype-build-pr22397-33fd5da.apk
Note: Google Login is not supported on these builds.

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Dec 4, 2025

App Icon📲 You can test the changes from this Pull Request in Jetpack by scanning the QR code below to install the corresponding build.
App NameJetpack
FlavorJalapeno
Build TypeDebug
Versionpr22397-33fd5da
Commit33fd5da
Direct Downloadjetpack-prototype-build-pr22397-33fd5da.apk
Note: Google Login is not supported on these builds.

@sonarqubecloud
Copy link

sonarqubecloud bot commented Dec 4, 2025

@nbradbury
Copy link
Contributor Author

The only suggestion is to clean up the now-unused SendButton composable in HENewTicketScreen.kt.

I made this change.

@nbradbury nbradbury marked this pull request as ready for review December 4, 2025 19:28
@codecov
Copy link

codecov bot commented Dec 4, 2025

Codecov Report

❌ Patch coverage is 0% with 11 lines in your changes missing coverage. Please review.
✅ Project coverage is 38.96%. Comparing base (2ae0e03) to head (33fd5da).

Files with missing lines Patch % Lines
...ndroid/support/he/ui/HEConversationDetailScreen.kt 0.00% 8 Missing ⚠️
...rdpress/android/support/he/ui/HENewTicketScreen.kt 0.00% 3 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##            trunk   #22397      +/-   ##
==========================================
- Coverage   38.96%   38.96%   -0.01%     
==========================================
  Files        2205     2205              
  Lines      106645   106649       +4     
  Branches    15085    15085              
==========================================
  Hits        41555    41555              
- Misses      61598    61602       +4     
  Partials     3492     3492              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants