You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
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.
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:
XML Layout (my_site_fragment_no_sites_view.xml:13): Removed the app:aevImage attribute from the ActionableEmptyView
Kotlin Fragment (MySiteFragment.kt:499-501): Added test code with TODO (as noted, this will be removed)
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:
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
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.
Do Not MergeIn PRs with this label, our automation will fail a require check, preventing accidental mergingMy Site
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
TODOregarding 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
