Skip to content

Conversation

@nbradbury
Copy link
Contributor

Fixes CMM-1043

In dark mode, the bookmark icon in the saved posts empty view should be lighter. This PR addresses this.

Before

dark

After

after

Here's the "after" shot in light mode:

after-light

@nbradbury
Copy link
Contributor Author

@claude

@claude
Copy link
Contributor

claude bot commented Dec 4, 2025

Claude finished @nbradbury's task —— View job


Code Review Complete ✓

I've reviewed this PR that improves the bookmark icon visibility in dark mode for the saved posts empty view. Here's my comprehensive analysis:


Summary

The implementation is solid and well-executed. The changes appropriately address the dark mode visibility issue by dynamically tinting the bookmark icon based on the theme's colorOnBackground attribute with reduced opacity (150/255 ≈ 59%).


Code Quality ✓

Strengths:

  • Clean, focused change that solves the specific problem
  • Proper use of Material theme attributes (MaterialR.attr.colorOnBackground) ensures consistency across themes
  • The getColorFromAttribute extension function is the correct approach for theme-aware coloring
  • Code is readable and follows Kotlin conventions

Minor observations:

  • The force unwrap (drawable!!) at line 1859 assumes the drawable is non-null after previous null-safe operations. While safe in practice (the drawable resource exists), the logic could be slightly more defensive:
    drawable?.let { d ->
        d.setTint(tint)
        d.alpha = 150
        d.setBounds(...)
        ssb.setSpan(ImageSpan(d), ...)
    }
    However, this is a minor point and the current approach is acceptable given the context.

Security ✓

No security concerns. The change only affects UI rendering using theme attributes.


Performance ✓

  • Negligible performance impact
  • Theme attribute resolution is cached by Android
  • The tinting operation is lightweight and only executed when displaying the empty view

Testing ⚠️

Observations:

  • No unit tests found for ReaderPostListFragment
  • The change affects UI rendering which is typically verified through:
    • Manual testing (screenshots provided in PR description ✓)
    • UI/screenshot tests (if available in the codebase)

Recommendation:
Consider verifying the change works correctly across:

  • Different device configurations (light/dark mode, different API levels)
  • Various screen densities
  • Accessibility modes (high contrast, etc.)

The manual testing screenshots in the PR description demonstrate light and dark mode work correctly, which is good.


Documentation ✓

  • The code is self-explanatory
  • No additional documentation needed for this UI fix
  • The PR description clearly explains the change with before/after screenshots

Best Practices ✓

Complies with repo guidelines:

  • ✓ Line length under 120 characters
  • ✓ No deprecated APIs used
  • ✓ Follows Kotlin coding conventions
  • ✓ Uses existing extension functions appropriately

Material Design compliance:

  • Using colorOnBackground is the correct Material attribute for content color
  • Alpha value of 150 (59% opacity) provides good visual hierarchy for secondary content

Suggestions for Consideration

  1. Alpha value as constant: Consider extracting the magic number 150 to a named constant for better maintainability:

    private const val BOOKMARK_ICON_ALPHA = 150
  2. Null safety: While not critical, using let scope function would make the null handling more explicit and avoid the force unwrap.


Verdict: APPROVED

This is a well-targeted fix that appropriately addresses the dark mode visibility issue. The implementation uses the correct Material Design patterns and integrates cleanly with the existing codebase. The suggestions above are minor enhancements and don't block approval.

Great work on providing clear before/after screenshots in the PR description! 🎯


@dangermattic
Copy link
Collaborator

dangermattic commented Dec 4, 2025

1 Warning
⚠️ PR is not assigned to a milestone.

Generated by 🚫 Danger

@nbradbury
Copy link
Contributor Author

  1. Consider extracting the magic number 150 to a named constant for better maintainability:

Done in 1af6db2.

@nbradbury nbradbury marked this pull request as ready for review December 4, 2025 21:23
@sonarqubecloud
Copy link

sonarqubecloud bot commented Dec 4, 2025

@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
Versionpr22399-6e108a5
Commit6e108a5
Direct Downloadjetpack-prototype-build-pr22399-6e108a5.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
Versionpr22399-6e108a5
Commit6e108a5
Direct Downloadwordpress-prototype-build-pr22399-6e108a5.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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants