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

[Reader] Update limit of fetched tag posts from 20 to 7 in Reader #19882

Conversation

RenanLukas
Copy link
Contributor

@RenanLukas RenanLukas commented Jan 3, 2024

Fixes #19472

This was previously implemented and then reverted because the item animations when paging happened were wrong.

It seems like #19881 fixed the animations issue, which made me implement this again since there's a meaningful performance impact for the user by decreasing the number of fetched posts.


To Test:

  • Install JP and sign in
  • Open Reader and select "Subscriptions" menu item;
  • Verify if 7 items are fetched each time we fetch a new page.

Another way to test is opening the tag posts list from blogging prompts card:

  • Install JP and sign in
  • Open "My Site"
  • Tap on "View all responses" in blogging prompts card
  • Verify if 7 items are fetched each time we fetch a new page.

Regression Notes

  1. Potential unintended areas of impact

    • Inserting new posts wrong animation
  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)

    --


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.

UI Changes Testing Checklist:

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

@RenanLukas RenanLukas marked this pull request as ready for review January 3, 2024 22:55
@wpmobilebot
Copy link
Contributor

wpmobilebot commented Jan 3, 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
Versionpr19882-3adf96a
Commit3adf96a
Direct Downloadjetpack-prototype-build-pr19882-3adf96a.apk
Note: Google Login is not supported on these builds.

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Jan 3, 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
Versionpr19882-3adf96a
Commit3adf96a
Direct Downloadwordpress-prototype-build-pr19882-3adf96a.apk
Note: Google Login is not supported on these builds.

@peril-wordpress-mobile
Copy link

Warnings
⚠️ PR is not assigned to a milestone.

Generated by 🚫 dangerJS

Base automatically changed from issue/19879-tag-posts-list-not-fetching-new-pages to feature/reader-ia January 8, 2024 19:43
@wordpress-mobile wordpress-mobile deleted a comment from wpmobilebot Jan 9, 2024
@daniloercoli daniloercoli self-assigned this Jan 12, 2024
@daniloercoli
Copy link
Contributor

We're correctly retrieving 7 items from the backend. 👍

A few things i noticed testing this PR:

Top Filter UI Element Reverts to Discovery

  • With a fresh installation of the app
  • Open the Reader, initially set to the Discover page.
  • Tap on Discover and switch to Subscriptions.
  • I observed that it quickly reverted back to Discover. Could this be due to an ongoing network request? or possibly because subscription data had not yet been retrieved?

** The animation expanding the Top UI is a bit laggy**

  • Transitioning from Discovery to Subscriptions on my device is slow, and briefly, I can see the first pill labeled "Subscriptio" until the UI fully expands.

Side note: is the Top UI element still in progress? If i select Subscriptions, then select "XX Tags", it does says "Selected Site".

@daniloercoli daniloercoli self-requested a review January 12, 2024 12:03
@RenanLukas
Copy link
Contributor Author

Thanks for the review, @daniloercoli

Top Filter UI Element Reverts to Discovery

I couldn't reproduce this one. I've tried in this branch and in the feature branch (feature/reader-ia), both with a fresh install.

Screen.Recording.2024-01-12.at.10.35.49.AM.mov

If it's reproducible and not caused by this change in the number of fetched posts (which at first sight doesn't seem related) I believe we should create a separate issue for it.

Transitioning from Discovery to Subscriptions on my device is slow, and briefly, I can see the first pill labeled "Subscriptio" until the UI fully expands.

I also couldn't reproduce this one. Does it happen regularly or just the first time Subscriptions is loaded? If it happens regularly maybe we should also create an issue.

Screen.Recording.2024-01-12.at.10.38.51.AM.mov

Side note: is the Top UI element still in progress? If i select Subscriptions, then select "XX Tags", it does says "Selected Site".

Hmm, this part should be done already. Where did you see the Selected Site label?

Screen.Recording.2024-01-12.at.10.43.48.AM.mov

@thomashorta
Copy link
Contributor

@daniloercoli I believe the branch of this PR is a bit outdated and many changes came in the feature/reader-ia since issue/19473-update-limit-of-fetched-tag-posts-after-fixing-paging was created, that's likely why you're seeing some of the issues you're seeing, and some of them were already fixed, like the one you mentioned here:

is the Top UI element still in progress? If i select Subscriptions, then select "XX Tags", it does says "Selected Site".

Since that Filter top bar UI component (the Blogs and Tags pills) was still in development when this branch was opened.

I just did a git push --force after rebasing this branch with the latest feature/reader-ia. Feel free to check again, as that should contain the latest changes related to the Reader IA project.

@thomashorta thomashorta force-pushed the issue/19473-update-limit-of-fetched-tag-posts-after-fixing-paging branch from 04cec98 to 3adf96a Compare January 12, 2024 20:25
@dangermattic
Copy link
Collaborator

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

Generated by 🚫 Danger

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.

LGTM! The issues I reported in the comments have either been already fixed or are not related to this PR. Consequently, I have opened new tickets for those that are unrelated.
#19938
#19939

@RenanLukas RenanLukas merged commit cd6fb60 into feature/reader-ia Jan 12, 2024
19 checks passed
@RenanLukas RenanLukas deleted the issue/19473-update-limit-of-fetched-tag-posts-after-fixing-paging branch January 12, 2024 21:01
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.

5 participants