Skip to content

CMM-1037 Home page picker fragment empty view - #22452

Merged
nbradbury merged 5 commits into
trunkfrom
issue/CMM-1037-HomePagePickerFragment-empty-view
Dec 23, 2025
Merged

CMM-1037 Home page picker fragment empty view#22452
nbradbury merged 5 commits into
trunkfrom
issue/CMM-1037-HomePagePickerFragment-empty-view

Conversation

@nbradbury

@nbradbury nbradbury commented Dec 22, 2025

Copy link
Copy Markdown
Contributor

As part of CMM-1037, this PR updates the empty view for the HomePagePickerFragment.

To test

  • Enable airplane mode
  • 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"
  • Skip choosing a theme
  • In the subsequent screen, verify the empty view appears correctly

Before and after shots

before

@nbradbury

Copy link
Copy Markdown
Contributor Author

@claude

@dangermattic

dangermattic commented Dec 22, 2025

Copy link
Copy Markdown
Collaborator
1 Warning
⚠️ PR is not assigned to a milestone.

Generated by 🚫 Danger

@claude

claude Bot commented Dec 22, 2025

Copy link
Copy Markdown
Contributor

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


Code Review Complete ✓

I've reviewed the changes in this PR. Overall, this is a clean and focused UI improvement that successfully simplifies the empty view presentation. Here's my comprehensive feedback:


Summary of Changes

This PR makes three related improvements to empty view displays:

  1. Home Page Picker Fragment - Removes image and landscape hiding behavior, changes height to match_parent for better centering
  2. History List Fragment - Removes image for consistency
  3. String Resources - Makes history empty state messages more concise and natural

Code Quality ✅

Strengths:

  • Clean, focused changes with a clear purpose
  • Consistent pattern applied across related screens (home page picker and history list)
  • The removal of aevImageHiddenInLandscape attribute simplifies the layout logic
  • Height change from wrap_content to match_parent ensures proper vertical centering

Visual Improvement:
The before/after screenshots show a significant improvement. The "After" version is cleaner, more focused, and better centered. Removing the illustration reduces visual clutter and puts more emphasis on the actionable message.


String Resource Improvements ✅

The string rewording is excellent:

  • Before: "When you make changes to your page you'll be able to see the history here"
  • After: "You'll see this page's history when you make changes to it"

This is more concise, more active, and reads more naturally. Good work!


Security ✅

No security concerns. These are UI-only changes to layout files and string resources.


Performance ✅

Positive impact:

  • Removing the image drawable (img_illustration_empty_results_216dp) means one less asset to load and render
  • Simpler layout hierarchy without image and conditional landscape logic
  • Should result in slightly faster rendering of empty states

Testing Considerations ⚠️

What's covered:

  • The PR description provides clear manual testing instructions
  • Visual regression is shown in the before/after screenshots

Potential gaps:

  1. Rotation testing: Since you removed aevImageHiddenInLandscape, it would be good to verify the empty view looks correct in landscape mode (though this is likely fine since there's no image anymore)
  2. Different screen sizes: Consider testing on various screen densities and sizes to ensure the centered text looks good everywhere
  3. Accessibility: Ensure the empty view is still accessible without the image (likely not an issue, but worth verifying with TalkBack)

These are minor - the testing instructions provided should be sufficient for this scope of change.


Documentation ✅

The PR description is clear and includes:

  • Detailed testing steps
  • Visual before/after comparison
  • Context linking to the Jira ticket (CMM-1037)

No additional documentation needed for this scope.


Additional Observations

  1. Consistency question: I notice you updated history_list_fragment.xml to also remove its image. Are there other fragments using ActionableEmptyView with this same illustration that should be updated for consistency? A quick search might reveal other candidates.

  2. Line length: The string changes are well within the 120 character limit specified in CLAUDE.md ✅

  3. Design pattern: This change establishes a cleaner pattern for empty states without illustrations. Consider documenting this as a standard for future empty views if it's part of a broader design direction.


Final Verdict

✅ Approved - This is a solid, well-executed UI improvement. The changes are clean, purposeful, and show clear visual improvement in the screenshots. The code quality is good, and there are no security or performance concerns.

The only minor suggestion is to verify the changes look good in landscape orientation and on various screen sizes, but these are low-risk given the simplicity of the changes.

Nice work, @nbradbury! 🎉


android:id="@+id/errorView"
android:layout_width="match_parent"
android:layout_height="wrap_content"
android:layout_height="match_parent"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good change! Using match_parent instead of wrap_content ensures the empty view is properly centered vertically within the available space. This matches the pattern shown in the "After" screenshot where the content is centered.

<string name="history_detail_title">Revision</string>
<string name="history_empty_subtitle_page">When you make changes to your page you\'ll be able to see the history here</string>
<string name="history_empty_subtitle_post">When you make changes to your post you\'ll be able to see the history here</string>
<string name="history_empty_subtitle_page">You\'ll see this page\'s history when you make changes to it</string>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice improvement to the string clarity! The new wording is more concise and active. The change from "When you make changes to your page you'll be able to see the history here" to "You'll see this page's history when you make changes to it" is clearer and more natural.

@nbradbury
nbradbury requested a review from adalpari December 22, 2025 21:16
@nbradbury
nbradbury marked this pull request as ready for review December 22, 2025 21:16
@sonarqubecloud

Copy link
Copy Markdown

@wpmobilebot

Copy link
Copy Markdown
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
Versionpr22452-15c3103
Commit15c3103
Direct Downloadjetpack-prototype-build-pr22452-15c3103.apk
Note: Google Login is not supported on these builds.

@wpmobilebot

Copy link
Copy Markdown
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
Versionpr22452-15c3103
Commit15c3103
Direct Downloadwordpress-prototype-build-pr22452-15c3103.apk
Note: Google Login is not supported on these builds.

@adalpari adalpari left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@nbradbury
nbradbury merged commit ddf4437 into trunk Dec 23, 2025
26 checks passed
@nbradbury
nbradbury deleted the issue/CMM-1037-HomePagePickerFragment-empty-view branch December 23, 2025 11:37
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