Reader site filter: show unseen post count #15581

ScoutHarris · 2021-01-05T20:03:01Z

On the Reader's site filter, a unseen post count is now displayed for each site.

To test:

Go to the Reader.
On any feed that has a filter (Following, Automattic, P2s), tap Filter.
Verify:
- For sites that have unseen posts, the count is displayed.
- For sites that have no unseen posts, the count is not displayed.
- The counts match those in the web Reader.
- Numbers above 1000 are abbreviated with multipliers(K, M, B, etc.)

To verify large numbers are abbreviated, you can hack the displayed count. In FilterProvider:addUnseenPostCount, change the displayed value: countLabel.text = topic.unseenCount.abbreviatedString()

Example:

let count = 876875666978
countLabel.text = count.abbreviatedString()

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.

peril-wordpress-mobile · 2021-01-05T20:03:09Z

You can trigger an installable build for these changes by visiting CircleCI here.

peril-wordpress-mobile · 2021-01-05T20:17:57Z

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

mattmiklic

These are looking good. Thanks @ScoutHarris!

ScoutHarris · 2021-01-05T20:53:16Z

Hey @mattmiklic . Sorry for the ping, but I just noticed something. Is the multiplier for thousands supposed to be upper or lower case "K"? I made it lowercase to match Android, but I see now the designs have uppercase. Thank you!

aerych

Heya @ScoutHarris !
I tested and the changes look good.
I had one nitpick code wise. Apart from that and the question about k vs K this should be good to go. :)

aerych · 2021-01-05T21:30:49Z

WordPress/Classes/ViewRelated/Stats/Extensions/Double+Stats.swift

@@ -43,7 +43,7 @@ extension Double {
                static let units: [Unit] = {
                    var units: [Unit] = []

-                    units.append(Unit(abbreviationFormat: NSLocalizedString("%@K", comment: "Label displaying value in thousands. Ex: 66.6K."), accessibilityLabelFormat: NSLocalizedString("%@ thousand", comment: "Accessibility label for value in thousands. Ex: 66.6 thousand.")))
+                    units.append(Unit(abbreviationFormat: NSLocalizedString("%@k", comment: "Label displaying value in thousands. Ex: 66.6k."), accessibilityLabelFormat: NSLocalizedString("%@ thousand", comment: "Accessibility label for value in thousands. Ex: 66.6 thousand.")))


Was going to ask if we wanted the upper case K to match the other abbreviations, but I see you already asked :)

I went ahead and reverted the change (i.e. make it uppercase again) since I think we all agree on it. If @mattmiklic rebels, I can change it later. 😄

No rebellion here, I agree with making this uppercase. :)

aerych · 2021-01-05T21:33:18Z

WordPress/Classes/ViewRelated/Reader/Filter/FilterProvider.swift

+    /// Adds a custom accessory view displaying the unseen post count.
+    ///
+    private static func addUnseenPostCount(_ topic: ReaderSiteTopic, with cell: UITableViewCell) {
+


Wondering if it would be better to set the accessory view to nil here. Refs my comment in prepareForReuse.

aerych · 2021-01-05T21:35:37Z

WordPress/Classes/ViewRelated/Reader/Filter/FilterTableData.swift

@@ -43,6 +43,10 @@ class SiteTableViewCell: UITableViewCell, GhostableView {
        fatalError("init(coder:) has not been implemented")
    }

+    override func prepareForReuse() {
+        accessoryView = nil


I think we'd probably want to call super here. :)

However, I'm wondering if this would be better done elsewhere. Per the docs:

For performance reasons, you should only reset attributes of the cell that are not related to content, for example, alpha, editing, and selection state.

Changes that modify the view hierarchy seem like they would be content related.

I think we'd probably want to call super here.

Every. Time. 🤦

I'm wondering if this would be better done elsewhere.

Thanks for that! Tis been moved.

ScoutHarris · 2021-01-05T22:12:39Z

Thanks @aerych ! Ready for another pass please.

aerych

Shiny!

ScoutHarris added 2 commits January 5, 2021 12:57

Show unseen count in custom accessory view.

b7f5b4c

Fix Dynamic Type build warning.

f04f1b4

ScoutHarris added [Type] Enhancement Reader labels Jan 5, 2021

ScoutHarris added this to the 16.5 milestone Jan 5, 2021

ScoutHarris self-assigned this Jan 5, 2021

Add release note.

a8a2f2b

ScoutHarris added the [Status] Needs Design Review A designer needs to sign off on the implemented design. label Jan 5, 2021

ScoutHarris requested review from aerych and mattmiklic January 5, 2021 20:29

ScoutHarris marked this pull request as ready for review January 5, 2021 20:30

ScoutHarris mentioned this pull request Jan 5, 2021

Reader: Add unread count for posts #15483

Closed

6 tasks

mattmiklic approved these changes Jan 5, 2021

View reviewed changes

mattmiklic removed the [Status] Needs Design Review A designer needs to sign off on the implemented design. label Jan 5, 2021

Merge branch 'develop' into 15483-show_unseen_count

df0e44f

aerych requested changes Jan 5, 2021

View reviewed changes

ScoutHarris added 2 commits January 5, 2021 15:08

Reset accessory view in addUnseenPostCount instead of prepareForReuse.

32b5bd7

Revert thousands multiplier to uppercase K.

2802a9c

ScoutHarris requested a review from aerych January 5, 2021 22:12

aerych approved these changes Jan 5, 2021

View reviewed changes

ScoutHarris merged commit e1f80fc into develop Jan 5, 2021

ScoutHarris deleted the 15483-show_unseen_count branch January 5, 2021 22:41

khaykov mentioned this pull request Jan 7, 2021

Issue/uppercase thousands suffix wordpress-mobile/WordPress-Android#13723

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reader site filter: show unseen post count #15581

Reader site filter: show unseen post count #15581

ScoutHarris commented Jan 5, 2021 •

edited

Loading

peril-wordpress-mobile bot commented Jan 5, 2021 •

edited

Loading

peril-wordpress-mobile bot commented Jan 5, 2021 •

edited

Loading

mattmiklic left a comment

ScoutHarris commented Jan 5, 2021 •

edited

Loading

aerych left a comment

aerych Jan 5, 2021

ScoutHarris Jan 5, 2021

mattmiklic Jan 6, 2021

aerych Jan 5, 2021

aerych Jan 5, 2021

ScoutHarris Jan 5, 2021

ScoutHarris commented Jan 5, 2021

aerych left a comment

Reader site filter: show unseen post count #15581

Reader site filter: show unseen post count #15581

Conversation

ScoutHarris commented Jan 5, 2021 • edited Loading

peril-wordpress-mobile bot commented Jan 5, 2021 • edited Loading

peril-wordpress-mobile bot commented Jan 5, 2021 • edited Loading

mattmiklic left a comment

Choose a reason for hiding this comment

ScoutHarris commented Jan 5, 2021 • edited Loading

aerych left a comment

Choose a reason for hiding this comment

aerych Jan 5, 2021

Choose a reason for hiding this comment

ScoutHarris Jan 5, 2021

Choose a reason for hiding this comment

mattmiklic Jan 6, 2021

Choose a reason for hiding this comment

aerych Jan 5, 2021

Choose a reason for hiding this comment

aerych Jan 5, 2021

Choose a reason for hiding this comment

ScoutHarris Jan 5, 2021

Choose a reason for hiding this comment

ScoutHarris commented Jan 5, 2021

aerych left a comment

Choose a reason for hiding this comment

ScoutHarris commented Jan 5, 2021 •

edited

Loading

peril-wordpress-mobile bot commented Jan 5, 2021 •

edited

Loading

peril-wordpress-mobile bot commented Jan 5, 2021 •

edited

Loading

ScoutHarris commented Jan 5, 2021 •

edited

Loading