[Reader] Remove improvements FF and unused UI files #20947
Conversation
Generated by 🚫 Danger |
|
 |
App Name | |
| Flavor | Jalapeno | |
| Build Type | Debug | |
| Version | pr20947-229ddd5 | |
| Commit | 229ddd5 | |
| Direct Download | jetpack-prototype-build-pr20947-229ddd5.apk |
|
 |
App Name | |
| Flavor | Jalapeno | |
| Build Type | Debug | |
| Version | pr20947-229ddd5 | |
| Commit | 229ddd5 | |
| Direct Download | wordpress-prototype-build-pr20947-229ddd5.apk |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## trunk #20947 +/- ##
==========================================
- Coverage 40.93% 40.77% -0.16%
==========================================
Files 1519 1518 -1
Lines 69554 69405 -149
Branches 11473 11463 -10
==========================================
- Hits 28473 28303 -170
- Misses 38495 38519 +24
+ Partials 2586 2583 -3 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
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?
| ) | ||
| } | ||
| } | ||
|
|
||
| @Suppress("LongParameterList") | ||
| fun mapPostToUiStateBlocking( |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
For some reason I thought it was being used, but it seems like it was just being used in the unit tests. Code updated.
| @@ -200,34 +196,12 @@ class ReaderExpandableTagsView @JvmOverloads constructor( | |||
| @ColorRes | |||
| fun overflowStrokeColorRes(isCollapsed: Boolean): Int? = null | |||
|
|
|||
| object Legacy : ChipStyle { | |||
| data object Default : ChipStyle { | |||
There was a problem hiding this comment.
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?
| // do nothing | ||
| } | ||
| } | ||
|
|
||
| class ImprovementsEnabled( |
There was a problem hiding this comment.
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?
| ReaderTagHeaderViewNewBinding.inflate(LayoutInflater.from(context), this, true) | ||
| val readerTagHeaderViewBinding = | ||
| ReaderTagHeaderViewBinding.inflate(LayoutInflater.from(context), this, true) | ||
| binding = | ||
| ReaderTagBinding.ImprovementsEnabled( |
There was a problem hiding this comment.
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).
…derPostDetailHeaderViewBinding directly
…iewBinding directly
|
Thanks for the review, @thomashorta ! |
|
Fixes #19261
To Test:
Basically the Reader should continue to work as expected, including all the feeds (
Discover,Subscriptions,Saved,Liked,Your Tagsand custom lists), post details, recommendation cards onDiscover, 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
What I did to test those areas of impact (or what existing automated tests I relied on)
What automated tests I added (or what prevented me from doing so)
ReaderDiscoverViewModelTest.kt.PR Submission Checklist:
RELEASE-NOTES.txtif necessary.Testing Checklist (strike-out the not-applying and unnecessary ones):