Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Tags IA] Integrate subfilters and post list in Tags Feed #20747

Merged

Conversation

thomashorta
Copy link
Contributor

@thomashorta thomashorta commented May 1, 2024

Fixes #20669

  • Refactor SubFilterViewModel to move relevant observers to ReaderFragment and be able to reuse it properly in Tags Feed
  • Fix subfilter in the top bar for Tags Feed
  • Implement the tag pills and "view more" feed button actions
  • Implement functionality to show the post list for selected/filtered tag

To Test:

  1. Make sure the reader-tags-feed feature flag is on
  2. Go to Reader
  3. Go to the Tags Feed
  4. Verify the top bar shows the "X Tags" pill
  5. Tap the filter chip in the top bar
  6. Verify it opens the filter bottom sheet
  7. Select a tag
  8. Verify it opens the Post List for that tag
  9. Clear the filter
  10. Verify it returns to the Tags Feed
  11. Tap the "header" pill for a tag in the feed
  12. Verify it opens the Post List for that tag
  13. Verify it updates de top bar subfilter pill
  14. Clear the filter
  15. Verify it returns to the Tags Feed
  16. Scroll horizontally to the end of a tag post row
  17. Tap "More from "
  18. Verify steps 12-15 happen

Regression Notes

  1. Potential unintended areas of impact

    • Breakage of top bar filters in other filterable screens (subscriptions/P2/automattic)
  2. What I did to test those areas of impact (or what existing automated tests I relied on)

    • Manual testing
  3. What automated tests I added (or what prevented me from doing so)

    • WIP

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):

  • WordPress.com sites and self-hosted Jetpack sites.
  • Portrait and landscape orientations.
  • Light and dark modes.
  • Fonts: Larger, smaller and bold text.
  • High contrast.
  • Talkback.
  • Languages with large words or with letters/accents not frequently used in English.
  • Right-to-left languages. (Even if translation isn’t complete, formatting should still respect the right-to-left layout)
  • Large and small screen sizes. (Tablet and smaller phones)
  • Multi-tasking: Split screen and Pop-up view. (Android 10 or higher)

Thomas Horta and others added 7 commits April 26, 2024 17:41
The idea is that the Provider is the component responsible for fetching the
correct SubFilterViewModel, but it doesn't necessarily own the ViewModel.

This was done so we can use the current feed as the Owner of the SubFilter
ViewModel without it actually declaring it, so the outermost layer of the Reader
(ReaderFragment) is still the source of truth for getting the SubFilterViewModel
but the actual lifecycle of the ViewModel is tied to the current feed fragment.
@dangermattic
Copy link
Collaborator

dangermattic commented May 1, 2024

2 Warnings
⚠️ This PR is larger than 300 lines of changes. Please consider splitting it into smaller PRs for easier and faster reviews.
⚠️ Class ActionEvent is missing tests, but unit-tests-exemption label was set to ignore this.

Generated by 🚫 Danger

@wpmobilebot
Copy link
Contributor

wpmobilebot commented May 1, 2024

Jetpack📲 You can test the changes from this Pull Request in Jetpack by scanning the QR code below to install the corresponding build.
App NameJetpack Jetpack
FlavorJalapeno
Build TypeDebug
Versionpr20747-306dc7c
Commit306dc7c
Direct Downloadjetpack-prototype-build-pr20747-306dc7c.apk
Note: Google Login is not supported on these builds.

@wpmobilebot
Copy link
Contributor

wpmobilebot commented May 1, 2024

WordPress📲 You can test the changes from this Pull Request in WordPress by scanning the QR code below to install the corresponding build.
App NameWordPress WordPress
FlavorJalapeno
Build TypeDebug
Versionpr20747-306dc7c
Commit306dc7c
Direct Downloadwordpress-prototype-build-pr20747-306dc7c.apk
Note: Google Login is not supported on these builds.

@thomashorta thomashorta marked this pull request as ready for review May 3, 2024 20:11
Copy link

codecov bot commented May 3, 2024

Codecov Report

Attention: Patch coverage is 40.90909% with 13 lines in your changes are missing coverage. Please review.

Project coverage is 40.76%. Comparing base (f6abbed) to head (306dc7c).
Report is 1 commits behind head on feature/tags-ia.

Files Patch % Lines
.../ui/reader/subfilter/SubFilterViewModelProvider.kt 35.71% 5 Missing and 4 partials ⚠️
...der/viewmodels/tagsfeed/ReaderTagsFeedViewModel.kt 50.00% 3 Missing and 1 partial ⚠️
Additional details and impacted files
@@               Coverage Diff                @@
##           feature/tags-ia   #20747   +/-   ##
================================================
  Coverage            40.75%   40.76%           
================================================
  Files                 1492     1493    +1     
  Lines                68505    68525   +20     
  Branches             11309    11315    +6     
================================================
+ Hits                 27920    27931   +11     
- Misses               38060    38065    +5     
- Partials              2525     2529    +4     

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

Copy link
Contributor

@daniloercoli daniloercoli left a comment

Choose a reason for hiding this comment

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

👍 The code is well-structured with clear separation of concerns.

I noticed that PTR doesn't work anymore in the Tags Feed.

Aside from this, the feature seems to work as expected. I've also noticed a couple of things about the new UI; I'm 99% sure they are not related to this PR, but I want to collect my feedback here since we're now working with a UI that uses real data:

  • Having to deal with horizontal lists of non-homogeneous items sometimes made me think there was a problem with the UI, even though it's not a problem of the UI itself; it's because the posts don't have pictures in them.

  • Scrolling horizontally through a row can be stuttering, and it's definitely not smooth on my device, a Pixel 5. It's not a new device, but it's not that old either.

  • I see the same problem with vertical scrolling in the feed. Follow a few tags, and try to scroll vertically. (The same happens when you follow new tags and go back to the list; you see the placeholders, and vertical scrolling is a bit stuttering.)

@thomashorta
Copy link
Contributor Author

thomashorta commented May 6, 2024

I noticed that PTR doesn't work anymore in the Tags Feed.

We didn't implement it yet, so it never worked in the Tags Feed. There's an issue opened for implementing that: #20707

Regarding the stuttering, right now the tag loading is far from optimal, since we're requesting everything at once and I'm not even sure what's running on the UI thread and what's actually being sent to a background thread. We still have other tasks to improve that, like this: #20708 Thanks for bringing it up!

@thomashorta
Copy link
Contributor Author

I just added the unit-tests-exemption since dangermattic is complaining about the ActionEvent class in the ReaderTagsFeedViewModel but @RenanLukas is actually working on more actions in #20670 which will likely change how things work regarding actions, therefore I will leave the tests for that OpenTagPostsFeed untested for now.

Copy link

sonarcloud bot commented May 6, 2024

Quality Gate Passed 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

@daniloercoli
Copy link
Contributor

I noticed that PTR doesn't work anymore in the Tags Feed.

We didn't implement it yet, so it never worked in the Tags Feed. There's an issue opened for implementing that: #20707

Regarding the stuttering, right now the tag loading is far from optimal, since we're requesting everything at once and I'm not even sure what's running on the UI thread and what's actually being sent to a background thread. We still have other tasks to improve that, like this: #20708 Thanks for bringing it up!

Okay, thanks for the clarification; I didn't notice the ticket about PTR. Approving this PR!

@daniloercoli daniloercoli self-requested a review May 7, 2024 09:48
@RenanLukas RenanLukas merged commit 6b45cfb into feature/tags-ia May 7, 2024
20 checks passed
@RenanLukas RenanLukas deleted the issue/20669-implement-more-from-tag-action branch May 7, 2024 15:09
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.

None yet

5 participants