Skip to content

Conversation

@nbradbury
Copy link
Contributor

@nbradbury nbradbury commented Dec 23, 2025

As part of CMM-1037, this PR updates the empty view for the LayoutPickerPreviewFragment. I also changed the layout to be centered, and replaced the deprecated android:tint with app:tint.

To test

  • On My Site, tap the dropdown arrow at the top right to switch sites
  • When the site picker appears, tap the FAB
  • Tap "Add a site"
  • Tap "Create WordPress.com site"
  • Enable airplane mode
  • Choose a theme
  • In the subsequent screen, verify the empty view appears correctly

Before and after shots

before

@dangermattic
Copy link
Collaborator

dangermattic commented Dec 23, 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 23, 2025

Claude finished @nbradbury's task in 1m 26s —— View job


Code Review Progress

  • Read the changed layout file
  • Review changes against Android best practices
  • Check consistency with related empty view changes
  • Provide comprehensive feedback

Code Review Summary

I've reviewed the layout changes in this PR. The changes successfully align with the CMM-1037 initiative to standardize empty views across the app. Here's my comprehensive feedback:

✅ Positive Observations

  1. Consistent Pattern: The changes align well with related empty view updates in PR CMM-1037 Home page picker fragment empty view #22452 (home_page_picker_fragment.xml) - both removed the aevImage attribute to simplify empty views.

  2. Proper Centering: Changed android:layout_height="0dp" to match_parent on the errorView (lines 79-80), which ensures the empty view content is properly centered vertically.

  3. Clean Implementation: The ActionableEmptyView retains the essential attributes (button, subtitle, title) while removing unnecessary visual elements.

🔍 Code Quality Issues

1. Deprecated Attribute Usage (Line 32)

File: layout_picker_preview_fragment.xml:32

The backButton ImageButton uses the deprecated android:tint attribute:

<ImageButton
    android:id="@+id/backButton"
    ...
    android:tint="?attr/colorOnSurface" />

Issue: This was changed to app:tint="?attr/colorOnSurface" in line 33 of the current version, which is the correct, non-deprecated approach. However, I notice this change isn't documented in the PR description or commit message as part of the empty view work.

Recommendation: This is actually a good fix! The change from android:tint to app:tint is the proper way to tint ImageButton icons in modern Android. Consider mentioning this improvement in the PR description or commit message.

📝 Documentation & Testing

  1. Testing Instructions: The PR provides clear, step-by-step testing instructions with a specific scenario to trigger the empty view (airplane mode).

  2. Visual Verification: The "Before and after" screenshot helps reviewers understand the visual impact of removing the image and centering the content.

🎯 Consistency Check

Comparing with related PRs:

The implementation is consistent with the established pattern.

Overall Assessment

Status: ✅ LGTM (Looks Good To Me)

The code changes are clean, follow Android best practices, and align with the CMM-1037 empty view standardization effort. The bonus fix for the deprecated android:tint attribute is welcome. No blocking issues found.

Minor Suggestion: Consider adding a note about the android:tintapp:tint fix in the PR description for completeness.


@nbradbury
Copy link
Contributor Author

The change from android:tint to app:tint is the proper way to tint ImageButton icons in modern Android. Consider mentioning this improvement in the PR description or commit message.

Done!

@sonarqubecloud
Copy link

@nbradbury nbradbury marked this pull request as ready for review December 23, 2025 20:19
@wpmobilebot
Copy link
Contributor

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
Versionpr22454-67d62bf
Commit67d62bf
Direct Downloadjetpack-prototype-build-pr22454-67d62bf.apk
Note: Google Login is not supported on these builds.

@wpmobilebot
Copy link
Contributor

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
Versionpr22454-67d62bf
Commit67d62bf
Direct Downloadwordpress-prototype-build-pr22454-67d62bf.apk
Note: Google Login is not supported on these builds.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants