CMM-894 new support general improvements #22320
Conversation
|
Claude finished @adalpari's task —— View job Code Review Complete ✅I've performed a comprehensive review of the PR changes. Overall, this is a solid improvement to the support feature with excellent accessibility enhancements. I've also added inline comments on specific lines for key issues. ✅ StrengthsAccessibility (a11y)
User Experience
Code Quality
🔍 Issues & Suggestions1. Error State Image may be Jetpack-Specific 💬Location: Using 2. Inconsistent Semantic DescriptionsLocation: In HE screen: val messageDescription = "${message.authorName}, $timestamp. ${message.formattedText}"In AI Bot screen: val author = if (message.isWrittenByUser) "You" else "AI Bot"
val messageDescription = "$author, $timestamp. ${message.formattedText}"Issue: Different patterns for constructing message descriptions. The AI Bot version is more user-friendly with "You" instead of the user's name. Suggestion: Consider standardizing this across both screens. Using "You" for user messages is more natural for screen readers. 3. Hardcoded Emoji in UILocation: Hardcoded emoji (✨) might not render consistently across all Android versions/devices. Consider using a drawable resource or at least add a comment documenting this design choice. 4. Missing Error Handling for Network State Changes 💬Location: The network check happens only during initial load. If the network becomes unavailable during use, the user might see generic errors instead of the offline view. Consider monitoring network state changes and updating 5. Potential Memory Leak in Typing AnimationLocation: The 6. Accessibility: Missing Role SemanticsLocation: Multiple files While content descriptions are excellent, some interactive elements could benefit from explicit role semantics (e.g., Suggestion: Consider adding role semantics to buttons and clickable cards: .semantics {
role = Role.Button
contentDescription = "..."
}7. Potential Race Condition in Message SendingLocation: The 8. String Concatenation in Accessibility DescriptionsLocation: Multiple locations, e.g., String concatenation with hardcoded punctuation (". ") might not work well for all languages. Consider using a dedicated string resource for combined descriptions to support proper localization: <string name="he_support_header_description">%1$s. %2$s</string>🧪 Testing Recommendations
📋 Minor Nitpicks
🎯 SummaryOverall Assessment: ✅ Approve with minor suggestions This PR demonstrates excellent attention to accessibility and user experience. The code is well-structured, properly organized, and follows good Compose patterns. The issues identified are mostly minor improvements and edge cases rather than critical problems. Key Achievements:
Recommended Actions:
Great work on this PR! The improvements to accessibility and user experience are substantial and well-implemented. 💬 = Has inline comment with more details |
Generated by 🚫 Danger |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
WordPress/src/main/java/org/wordpress/android/support/common/ui/ErrorConversationsView.kt
Show resolved
Hide resolved
WordPress/src/main/java/org/wordpress/android/support/he/ui/HENewTicketScreen.kt
Show resolved
Hide resolved
WordPress/src/main/java/org/wordpress/android/support/he/ui/HEConversationDetailScreen.kt
Outdated
Show resolved
Hide resolved
| private suspend fun loadConversations() { | ||
| try { | ||
| _isLoadingConversations.value = true | ||
| if (!networkUtilsWrapper.isNetworkAvailable()) { |
There was a problem hiding this comment.
The network check happens only during initial load. Consider monitoring network state changes throughout the lifecycle to provide better offline detection. If the network becomes unavailable while the user is viewing conversations, they might see generic errors instead of the dedicated offline view.
|
| App Name | WordPress | |
| Flavor | Jalapeno | |
| Build Type | Debug | |
| Version | pr22320-865b0ee | |
| Commit | 865b0ee | |
| Direct Download | wordpress-prototype-build-pr22320-865b0ee.apk |
|
| App Name | Jetpack | |
| Flavor | Jalapeno | |
| Build Type | Debug | |
| Version | pr22320-865b0ee | |
| Commit | 865b0ee | |
| Direct Download | jetpack-prototype-build-pr22320-865b0ee.apk |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## feat/CMM-883-support-Oddie-bot-conversation-pagination #22320 +/- ##
==========================================================================================
- Coverage 39.36% 39.33% -0.04%
==========================================================================================
Files 2195 2198 +3
Lines 105296 105394 +98
Branches 14978 14983 +5
==========================================================================================
+ Hits 41454 41460 +6
- Misses 60351 60442 +91
- Partials 3491 3492 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
adalpari
changed the title
| .semantics(mergeDescendants = true) { | ||
| contentDescription = messageDescription | ||
| } |
There was a problem hiding this comment.
Currently, TalkBack announces the author, date, and message contents twice. This is less than ideal and can significantly increase the amount of noise when navigating long messages.
I believe the issue originates from combining mergeDescendants and contentDescription. We should likely do one of two things here and anywhere else we are taking this approach:
- Keep the
semantics(mergeDescendants = true)withcontentDescriptionon the Box. Add.semantics { invisibleToUser() }to both Text composables inside the Column. - Keep
semantics(mergeDescendants = true)but remove thecontentDescriptionassignment. Optionally prepend the author information to the message text itself.
The first option likely makes the most sense.
There was a problem hiding this comment.
Good catch! Fixing it!
| <string name="ai_bot_empty_conversations_message">Start a new conversation to get help with your WordPress site or account.</string> | ||
| <string name="ai_bot_empty_conversations_button">Start conversation</string> | ||
| <string name="ai_bot_you">You</string> | ||
| <string name="ai_bot_odie" translatable="false">Odie</string> |
There was a problem hiding this comment.
Do we surface the name "Odie" anywhere else? If not, this might be confusing. Should we use a generic "Bot" instead?
There was a problem hiding this comment.
mmm, good question. I'll figure it out.
|
adalpari
merged commit e066a74
into
feat/CMM-883-support-Oddie-bot-conversation-pagination
* Support pagination * Triggering in the 4th element * detekt * TODO for debug purposes * Claude PR suggestions Mutex and constant * Support pagination * Triggering in the 4th element * detekt * TODO for debug purposes * Claude PR suggestions Mutex and constant * Detekt * Removing testing code * CMM-894 new support general improvements (#22320) * Put ConversationListView in common between bots and HE * Empty and error state * Skip site capitalization * Adding a11c labels * Adding headings labels * adding accessible labels to chat bubbles * detekt * Fixing tests * PR suggestion about bot chat bubble * Fixing tests * Fixing TalkBack duplication
* Adding basic UI * Renaming * Some styling * Renaming and dummy data * Using proper "new conversation icon" * Conversation details screen * Creating the reply bottomsheet * Linking to the support screen * bottomsheet fix * Mov navigation form activity to viewmodel * Adding create ticket screen * More screen adjustments * Extracting common code * Margin fix * detekt * Style * New ticket check * Creating tests * Creating repository and load conversations function * Adding createConversation function * Creating loadConversation func * Loading conversations form the viewmodel * Adding loading spinner * Pull to refresh * Proper ionitialization * Adding empty screen * Handling send new conversation * Show loading when sending * New ticket creation fix * Using snackbar for errors * Error handling * Answering conversation * Adding some test to the repository * More tests! * Compile fixes * Similarities improvements * Using snackbar in bots activity * Extracting EmptyConversationsView * Renaming * Extracting VM and UI common code * Extracting navigation common code * Renaming VMs for clarification * More refactor * Capitalise text fields * Updating rs library * Loading conversation UX * Style fix * Fixing scaffolds paddings * userID fix * Fixing the padding problem in bot chat when the keyboard is opened * Apply padding to create ticket screen when the keyboard is opened * Fixing scroll state in reply bottomsheet * Adding tests for the new common viewmodel * Fixing AIBotSupportViewModel tests * detekt * Improvements int he conversation interaction * Adding tests for HE VM * Saving draft state * Properly navigating when a ticket is selected * Error parsing improvement * accessToken suggestion improvements * General suggestions * Send message error UX improvement * Fixing tests * Converting the UI to more AndroidMaterial style * Bots screen renaming * Bots screens renaming * Make NewTicket screen more Android Material theme as well * Adding preview for EmptyConversationsView * Button fix * detekt * Ticket selection change * Supporting markdown text * detekt * Improving MarkdownUtils * Formatting text in the repository layer instead the ui * Renaming * Fixing tests * Parsing markdown more exhaustively * New links support * Detekt * CMM-883 support Odie bot conversation pagination (#22316) * Support pagination * Triggering in the 4th element * detekt * TODO for debug purposes * Claude PR suggestions Mutex and constant * Support pagination * Triggering in the 4th element * detekt * TODO for debug purposes * Claude PR suggestions Mutex and constant * Detekt * Removing testing code * CMM-894 new support general improvements (#22320) * Put ConversationListView in common between bots and HE * Empty and error state * Skip site capitalization * Adding a11c labels * Adding headings labels * adding accessible labels to chat bubbles * detekt * Fixing tests * PR suggestion about bot chat bubble * Fixing tests * Fixing TalkBack duplication
Description
This PR is achieving different improvements suggested for the new Support:
- Site address is not capitalised anymore
- Site address now shows the URL-specific keyboard
- Better a11y input labels for a better SpeechToVoice navigation
- Application logs toggle a11y label
Testing instructions