Skip to content

Conversation

@nbradbury
Copy link
Contributor

As part of CMM-1037, this PR removes the image from the My Site fragment's empty view, which is shown when there are no sites.

Note1: I've added a TODO regarding test code which ensures that regardless of the account, the empty view appears. I'll remove this before merging. This is why the "Do not merge" label has been added.

Note2: I'm not convinced removing the image in this situation is the right thing to do. I know we're trying to get rid of outdated images in the various empty views, but not having any sites seems a special situation that may warrant keeping the image. Thoughts?

To test
Simply run the app and verify the empty view on the My Site page appears correctly.

Before and After
before

@nbradbury nbradbury added Do Not Merge In PRs with this label, our automation will fail a require check, preventing accidental merging My Site labels Jan 20, 2026
@dangermattic
Copy link
Collaborator

dangermattic commented Jan 20, 2026

1 Error
🚫 This PR is tagged with Do Not Merge label(s).
1 Warning
⚠️ PR is not assigned to a milestone.

Generated by 🚫 Danger

@sonarqubecloud
Copy link

@nbradbury
Copy link
Contributor Author

@claude Ignore the "TODO" and the "Do not merge" label.

@claude
Copy link
Contributor

claude bot commented Jan 20, 2026

Claude finished @nbradbury's task in 57s —— View job


Code Review

Reviewing PR changes (ignoring TODO and "Do not merge" label as requested).

Tasks

  • Read changed files to understand modifications
  • Perform comprehensive code review
  • Provide detailed feedback

Review Summary

I've reviewed the changes in this PR. The modifications are straightforward and handle the image removal correctly. Here are my findings:

✅ Code Quality - Excellent

The changes are minimal and focused:

  1. XML Layout (my_site_fragment_no_sites_view.xml:13): Removed the app:aevImage attribute from the ActionableEmptyView
  2. Kotlin Fragment (MySiteFragment.kt:499-501): Added test code with TODO (as noted, this will be removed)
  3. Code formatting fix (MySiteFragment.kt:503): Fixed spacing in the if statement

The ActionableEmptyView component handles missing images gracefully - when imageResource == 0, it simply doesn't set or show the image (ActionableEmptyView.kt:85-90). No null pointer or visibility issues will occur.

✅ Security - No Issues

No security concerns. This is a UI-only change with no security implications.

✅ Performance - No Issues

Removing the image actually improves performance slightly by:

  • Reducing layout inflation time
  • Eliminating image loading operations
  • Reducing memory usage

⚠️ Testing - Minor Observation

The test code (lines 499-501) forces the empty view to always display, which is intentional for testing purposes. This correctly simulates the "no sites" state regardless of actual account status.

Note: As mentioned in the PR description, this TODO code will be removed before merge.

📝 Documentation - Consider Adding Context

While the code changes are correct, you might want to consider:

  1. Design Decision Documentation: Since you mentioned uncertainty about removing the image ("not convinced this is the right thing to do"), it might be helpful to document the reasoning if this gets merged. This could be:

    • A comment in the layout file explaining why no image is used
    • Or a note in the commit message for future reference
  2. Related Empty Views: If this is part of a broader initiative to remove outdated images from empty views (as mentioned in the PR description), you might want to ensure consistency across similar empty states.

Code Style Compliance

The changes follow the project's code style guidelines:

  • ✅ Line length under 120 characters
  • ✅ Proper Kotlin formatting
  • ✅ No deprecated APIs used
  • ✅ Uses TODO instead of FIXME

Conclusion

The implementation is clean and correct. The ActionableEmptyView component was designed to handle optional images, so removing the app:aevImage attribute is safe and won't cause any runtime issues. The test code clearly marked with TODO serves its purpose for verification.

The only open question is the design decision itself (whether to remove the image), which you've already flagged for discussion. From a technical implementation standpoint, this PR is solid.


@nbradbury nbradbury marked this pull request as ready for review January 20, 2026 17:31
@nbradbury nbradbury requested a review from adalpari January 20, 2026 17:31
@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
Versionpr22503-633a812
Commit633a812
Direct Downloadjetpack-prototype-build-pr22503-633a812.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
Versionpr22503-633a812
Commit633a812
Direct Downloadwordpress-prototype-build-pr22503-633a812.apk
Note: Google Login is not supported on these builds.

@wpmobilebot
Copy link
Contributor

🤖 Build Failure Analysis

This build has failures. Claude has analyzed them - check the build annotations for details.

@nbradbury
Copy link
Contributor Author

This build has failures.

This is due to the "Do not merge" label.

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

Labels

Do Not Merge In PRs with this label, our automation will fail a require check, preventing accidental merging My Site

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants