[Reader] Remove improvements FF and unused UI files #20947

RenanLukas · 2024-06-06T20:51:02Z

Fixes #19261

To Test:

Basically the Reader should continue to work as expected, including all the feeds (Discover, Subscriptions, Saved, Liked, Your Tags and custom lists), post details, recommendation cards on Discover, actions, etc.
It would be nice to also test it out using light and dark themes since the PR is related to UI.

Regression Notes

Potential unintended areas of impact
- Reader post lists and post details.
What I did to test those areas of impact (or what existing automated tests I relied on)
- Manual testing
What automated tests I added (or what prevented me from doing so)
- Updated ReaderDiscoverViewModelTest.kt.

PR Submission Checklist:

I have completed the Regression Notes.
I have considered adding accessibility improvements for my changes.
I have considered if this change warrants user-facing release notes and have added them to RELEASE-NOTES.txt if necessary.

Testing Checklist (strike-out the not-applying and unnecessary ones):

dangermattic · 2024-06-06T20:53:44Z

	4 Warnings
⚠️	View files have been modified, but no screenshot or video is included in the pull request. Consider adding some for clarity.
⚠️	This PR is larger than 300 lines of changes. Please consider splitting it into smaller PRs for easier and faster reviews.
⚠️	Class `ChipStyle` is missing tests, but `unit-tests-exemption` label was set to ignore this.
⚠️	PR is not assigned to a milestone.

Generated by 🚫 Danger

wpmobilebot · 2024-06-06T21:10:28Z

📲 You can test the changes from this Pull Request in Jetpack by scanning the QR code below to install the corresponding build.

	App Name	Jetpack
	Flavor	Jalapeno
	Build Type	Debug
	Version	pr20947-5726d0d
	Commit	`5726d0d`
	Direct Download	`jetpack-prototype-build-pr20947-5726d0d.apk`

Note: Google Login is not supported on these builds.

wpmobilebot · 2024-06-06T21:11:14Z

📲 You can test the changes from this Pull Request in WordPress by scanning the QR code below to install the corresponding build.

	App Name	WordPress
	Flavor	Jalapeno
	Build Type	Debug
	Version	pr20947-5726d0d
	Commit	`5726d0d`
	Direct Download	`wordpress-prototype-build-pr20947-5726d0d.apk`

Note: Google Login is not supported on these builds.

codecov · 2024-06-07T01:12:04Z

Codecov Report

Attention: Patch coverage is 85.29412% with 5 lines in your changes missing coverage. Please review.

Project coverage is 40.41%. Comparing base (5f0bc7d) to head (5726d0d).
Report is 20 commits behind head on trunk.

Files with missing lines	Patch %	Lines
...oid/ui/reader/discover/ReaderPostUiStateBuilder.kt	72.72%	2 Missing and 1 partial ⚠️
...der/viewmodels/tagsfeed/ReaderTagsFeedViewModel.kt	0.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##            trunk   #20947      +/-   ##
==========================================
- Coverage   40.57%   40.41%   -0.16%     
==========================================
  Files        1535     1534       -1     
  Lines       70516    70367     -149     
  Branches    11662    11652      -10     
==========================================
- Hits        28609    28439     -170     
- Misses      39322    39346      +24     
+ Partials     2585     2582       -3

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

thomashorta

Overall the code looks good but I noticed a few places that the New nomenclature is still being used (in some class names and Theme styles) and also found a place where we are mapping to a "legacy" UI model and I'm not sure if that means we're still showing the old post UI somewhere, can you double check?

thomashorta · 2024-06-10T21:00:37Z

WordPress/src/main/java/org/wordpress/android/ui/reader/discover/ReaderPostUiStateBuilder.kt

-            )
-        }
-    }
-
    @Suppress("LongParameterList")
    fun mapPostToUiStateBlocking(


It looks like this is still being used somewhere, is that correct? Doesn't it map to the old UI State for the old post UI?

For some reason I thought it was being used, but it seems like it was just being used in the unit tests. Code updated.

thomashorta · 2024-06-10T21:09:08Z

WordPress/src/main/java/org/wordpress/android/ui/reader/views/ReaderExpandableTagsView.kt

@@ -200,34 +196,12 @@ class ReaderExpandableTagsView @JvmOverloads constructor(
        @ColorRes
        fun overflowStrokeColorRes(isCollapsed: Boolean): Int? = null

-        object Legacy : ChipStyle {
+        data object Default : ChipStyle {


Now that we have just one implementation, what do you think about making ChipStyle a concrete class and moving the implementation from this subclass to there?

thomashorta · 2024-06-10T21:09:44Z

WordPress/src/main/java/org/wordpress/android/ui/reader/views/ReaderPostDetailHeaderView.kt

-                // do nothing
-            }
-        }
-
        class ImprovementsEnabled(


Now that we have just one implementation, what do you think about making ReaderPostDetailHeaderBinding a concrete class and moving the implementation from this subclass to there?

thomashorta · 2024-06-10T21:13:24Z

WordPress/src/main/java/org/wordpress/android/ui/reader/views/ReaderTagHeaderView.kt

-                ReaderTagHeaderViewNewBinding.inflate(LayoutInflater.from(context), this, true)
+        val readerTagHeaderViewBinding =
+            ReaderTagHeaderViewBinding.inflate(LayoutInflater.from(context), this, true)
+        binding =
            ReaderTagBinding.ImprovementsEnabled(


Now that we have just one implementation, what do you think about making ReaderTagBinding a concrete class and moving the implementation from this subclass to there?

Also, delete the now unused ImprovementsDisabled class (which will be necessary anyway if you implement the suggestion above).

…ault subclass

…derPostDetailHeaderViewBinding directly

…iewBinding directly

RenanLukas · 2024-06-10T23:44:54Z

Thanks for the review, @thomashorta !
I've implemented your suggestions and if CI is 🟢 it should be ready for a second review.

sonarcloud · 2024-06-11T14:59:30Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

sonarcloud · 2024-08-28T18:21:30Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

nbradbury

As discussed elsewhere, we've decided to close older PRs that have non-trivial merge conflicts and/or have non-trivial CI issues. Based on that, this should be closed. However, given that this removes 3001 lines of outdated code and it was previously approved, I felt it worthwhile to update it and get it working.

RenanLukas added 7 commits June 5, 2024 18:22

Remove reader improvements feature config

7984511

Remove feature flag logic

afe6bae

Remove unused UI

28e0af0

Fix ReaderPostUiStateBuilder unit tests

a3ea9f7

Fix detekt

79ab3a8

Remove unused files and new suffix

0827d2a

Continue removing reader unused files and new suffix

8d71310

RenanLukas added 2 commits June 6, 2024 18:17

Fix lint

5cd7e4a

Remove unused reader styles

f933e21

RenanLukas added Reader Reader Improvements labels Jun 6, 2024

RenanLukas marked this pull request as ready for review June 6, 2024 23:00

RenanLukas requested review from thomashorta and develric June 6, 2024 23:00

Fix ReaderDiscoverViewModel unit tests

64db899

thomashorta reviewed Jun 10, 2024

View reviewed changes

RenanLukas added 6 commits June 10, 2024 18:55

Apply PR suggestion: make ChipStyle a concrete class and deleting Def…

57fcdf8

…ault subclass

Apply PR suggestion: remove ReaderPostDetailHeaderBinding and use Rea…

ecd22e1

…derPostDetailHeaderViewBinding directly

Remove unused onDiscoverClicked

5b49f7d

Apply PR suggestion: remove ReaderTagBinding and use ReaderTagHeaderV…

ddf7bb5

…iewBinding directly

Fix detekt

bba2471

Apply PR suggestion: remove old UI logic from ReaderPostAdapter

76aa4c3

RenanLukas added the unit-tests-exemption label Jun 10, 2024

Fix lint

229ddd5

thomashorta approved these changes Jun 14, 2024

View reviewed changes

jkmassel removed Reader Improvements labels Jul 3, 2024

Merge branch 'trunk' into issue/19261-reader-remove-unused-files

82d05ce

nbradbury self-assigned this Aug 28, 2024

Fixed string formatting issue

5726d0d

nbradbury added the unit-tests-exemption label Aug 28, 2024

nbradbury requested review from nbradbury and removed request for develric August 28, 2024 19:06

nbradbury approved these changes Aug 28, 2024

View reviewed changes

nbradbury merged commit 9ee898d into trunk Aug 28, 2024
21 checks passed

nbradbury deleted the issue/19261-reader-remove-unused-files branch August 28, 2024 19:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Reader] Remove improvements FF and unused UI files #20947

[Reader] Remove improvements FF and unused UI files #20947

RenanLukas commented Jun 6, 2024 •

edited

Loading

dangermattic commented Jun 6, 2024 •

edited

Loading

wpmobilebot commented Jun 6, 2024 •

edited

Loading

wpmobilebot commented Jun 6, 2024 •

edited

Loading

codecov bot commented Jun 7, 2024 •

edited

Loading

thomashorta left a comment •

edited

Loading

thomashorta Jun 10, 2024

RenanLukas Jun 10, 2024

thomashorta Jun 10, 2024

thomashorta Jun 10, 2024

thomashorta Jun 10, 2024

RenanLukas commented Jun 10, 2024

sonarcloud bot commented Jun 11, 2024

sonarcloud bot commented Aug 28, 2024

nbradbury left a comment •

edited

Loading

[Reader] Remove improvements FF and unused UI files #20947

[Reader] Remove improvements FF and unused UI files #20947

Conversation

RenanLukas commented Jun 6, 2024 • edited Loading

To Test:

Regression Notes

PR Submission Checklist:

Testing Checklist (strike-out the not-applying and unnecessary ones):

dangermattic commented Jun 6, 2024 • edited Loading

wpmobilebot commented Jun 6, 2024 • edited Loading

wpmobilebot commented Jun 6, 2024 • edited Loading

codecov bot commented Jun 7, 2024 • edited Loading

Codecov Report

thomashorta left a comment • edited Loading

Choose a reason for hiding this comment

thomashorta Jun 10, 2024

Choose a reason for hiding this comment

RenanLukas Jun 10, 2024

Choose a reason for hiding this comment

thomashorta Jun 10, 2024

Choose a reason for hiding this comment

thomashorta Jun 10, 2024

Choose a reason for hiding this comment

thomashorta Jun 10, 2024

Choose a reason for hiding this comment

RenanLukas commented Jun 10, 2024

sonarcloud bot commented Jun 11, 2024

Quality Gate passed

sonarcloud bot commented Aug 28, 2024

Quality Gate passed

nbradbury left a comment • edited Loading

Choose a reason for hiding this comment

RenanLukas commented Jun 6, 2024 •

edited

Loading

dangermattic commented Jun 6, 2024 •

edited

Loading

wpmobilebot commented Jun 6, 2024 •

edited

Loading

wpmobilebot commented Jun 6, 2024 •

edited

Loading

codecov bot commented Jun 7, 2024 •

edited

Loading

thomashorta left a comment •

edited

Loading

nbradbury left a comment •

edited

Loading