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

Adding unread posts count to Reader bottom sheet #13625

Merged
merged 12 commits into from
Jan 7, 2021

Conversation

develric
Copy link
Contributor

@develric develric commented Dec 17, 2020

Fixes #13454

This adds unread posts count to the reader filter bottom sheet. Note that to align the reader subfilter to the iOS implementation we removed the selected item checkmark in this #13648

The information about the unread posts is contained in the endpoint read/following/mine from v1.2 API, so we needed to upgrade the rest client from v1.1 for this call.

The following images show the implemented result both in light and dark mode.

Light mode Dark mode
image image

Note 1: please note that the update of the counters is async so sometimes is possible you need to Pull To Refresh before to get the updated numbers in the bottom sheet

Note 2: to see the counters in Calypso, it is possible you need to use a a8c account

Note 3: currently when a post is read from the app is not marked as seen so the counters are not updated by this action on the app. There is an task that will land in a separate PR to add this enhancement in the app reader.

Note 4: a small deviation from design is that I kept 16dp right edge as the left side in the bottom sheet instead of the 14dp (pinging @mattmiklic for a quick check on this and other things he may notice eventually 🙇 )

To test

The feature is behind the UNREAD_POSTS_COUNT feature flag. You need to build the wasabiDebug flavor from sources or use the APK from this PR CI build remembering to activate the feature in App Settings -> Test Feature Configuration (IMPORTANT: remember to restart the app after changing these flags configuration)

Checking the feature

  • Login into the app with an a8c account (in principle you can also login with a not a8c account but this step is needed to be able to compare the counters with Calypso; see Note 2 above)
  • Go to the reader and check the counters are shown
  • Using the same account as for the app, go to https://wordpress.com/read and check the counters are coherent
WP Android app Web reader
image image
  • Try to publish one or more post, refresh the app and the web and check the numbers stay consistent

Checking big numbers abbreviation

  • Numbers above 1000 are abbreviated with multipliers like k, M etc...
  • In AS place an Evaluate and Log breakpoint like below in SiteViewHolder Line 35 with this config this.itemUnseenCount.text = statsUtils.toFormattedString(123456L, ONE_THOUSAND)

  • Connect to the app with the debugger and check you get expected abbreviation (you can also change the number 123456L in the breakpoint to test other number ranges)

Smoke testing without the feature enabled

  • Disable the feature flag in App Settings -> Test Feature Configuration and restart the app
  • smoke test the reader and the filter bottom sheet

PR submission checklist:

  • I have considered adding unit tests where possible.
  • 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.

@develric develric added this to the 16.5 milestone Dec 17, 2020
@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Dec 17, 2020

You can trigger optional UI/connected tests for these changes by visiting CircleCI here.

@peril-wordpress-mobile
Copy link

peril-wordpress-mobile bot commented Dec 17, 2020

You can test the changes on this Pull Request by downloading the APK here.

# Conflicts:
#	WordPress/src/main/java/org/wordpress/android/datasets/ReaderBlogTable.java
#	WordPress/src/main/java/org/wordpress/android/datasets/ReaderDatabase.java
#	WordPress/src/main/java/org/wordpress/android/models/ReaderBlog.java
…ue/13454-reader-unread-posts-count

# Conflicts:
#	WordPress/src/main/res/layout/subfilter_list_item.xml
@develric develric changed the base branch from develop to issue/13648-remove-checkmark-reader-subfilter December 21, 2020 23:13
Base automatically changed from issue/13648-remove-checkmark-reader-subfilter to develop December 22, 2020 07:53
@develric develric marked this pull request as ready for review December 22, 2020 17:34
@develric develric marked this pull request as draft December 22, 2020 17:54
@mattmiklic
Copy link
Member

This looks good to me, thanks @develric!

@develric develric marked this pull request as ready for review January 4, 2021 17:03
@renanferrari
Copy link
Member

Hey @develric, sorry for the delay on this one. I'll make sure to take a look at it by tomorrow, if no one gets to it before me :)

Copy link
Member

@renanferrari renanferrari left a comment

Choose a reason for hiding this comment

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

Thanks for the changes @develric! I've tested the app and everything seems to be working as expected. Code also looks pretty good – I just left one comment below, but it's pretty minor.

@@ -81,6 +82,10 @@ public static ReaderBlog fromJson(JSONObject json) {
blog.numSubscribers = json.optInt("subscribers_count");
}

if (json.has("unseen_count")) {
blog.numUnseenPosts = json.optInt("unseen_count");
Copy link
Member

Choose a reason for hiding this comment

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

Checking that the parameter exists with has() doesn't seem to be necessary here, as we're using optInt() which fallbacks to 0 by default.

@develric
Copy link
Contributor Author

develric commented Jan 7, 2021

Hey @renanferrari 👋 , thanks so much for the review 🙇 ! I agree with the np, since you already approved this one and I need to create a dedicated PR to change the multiplier from lower case k to upper case K I'm going to merge this one and will manage it in the new PR. Thanks again 😄

@develric develric merged commit fa0b383 into develop Jan 7, 2021
@develric develric deleted the issue/13454-reader-unread-posts-count branch January 7, 2021 16:52
@khaykov khaykov mentioned this pull request Jan 7, 2021
3 tasks
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.

Add unread post count to Reader
3 participants