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.

Sign up for GitHub

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

Jump to bottom

Stats: Total Subscribers Card #23084

Merged
merged 7 commits into from Apr 30, 2024
Merged

Stats: Total Subscribers Card #23084

merged 7 commits into from Apr 30, 2024

Conversation

staskus
Copy link
Contributor

@staskus staskus commented Apr 25, 2024 •
edited

Fixes #23048

image

Solution

  1. Updated subscribersList to return both subscribers and subscribers totals since they come from the same endpoint
  2. Added Total Subscribers card, reusing the same UI from Insights

Note: Insights Totals Subscribers card may show different numbers since it also adds publicize numbers. Fix is made here #23099

To test:

  1. Enable Stats Traffic and Subscribers tab feature flag
  2. Open Stats -> Subscribers
  3. Confirm Total Subscribers card appears
  4. Compare the number with the web

Regression Notes

  1. Potential unintended areas of impact

None

  1. What I did to test those areas of impact (or what existing automated tests I relied on)

  2. What automated tests I added (or what prevented me from doing so)

Added unit tests

PR submission checklist:

  • I have completed the Regression Notes.
  • I have considered adding unit tests for my changes.
  • 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:

  • WordPress.com sites and self-hosted Jetpack sites.
  • Portrait and landscape orientations.
  • Light and dark modes.
  • Fonts: Larger, smaller and bold text.
  • High contrast.
  • VoiceOver.
  • 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)
  • iPhone and iPad.
  • Multi-tasking: Split view and Slide over. (iPad)

@staskus staskus added this to the 24.8 milestone Apr 25, 2024
@staskus staskus requested a review from guarani April 25, 2024 09:53
@wpmobilebot
Copy link
Contributor

wpmobilebot commented Apr 25, 2024 •
edited

Jetpack Alpha📲 You can test the changes from this Pull Request in Jetpack Alpha by scanning the QR code below to install the corresponding build.
App NameJetpack Alpha Jetpack Alpha
ConfigurationRelease-Alpha
Build Numberpr23084-c307735
Version24.7
Bundle IDcom.jetpack.alpha
Commitc307735
App Center Buildjetpack-installable-builds #8777
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

@wpmobilebot
Copy link
Contributor

wpmobilebot commented Apr 25, 2024 •
edited

WordPress Alpha📲 You can test the changes from this Pull Request in WordPress Alpha by scanning the QR code below to install the corresponding build.
App NameWordPress Alpha WordPress Alpha
ConfigurationRelease-Alpha
Build Numberpr23084-c307735
Version24.7
Bundle IDorg.wordpress.alpha
Commitc307735
App Center BuildWPiOS - One-Offs #9732
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

staskus
staskus commented Apr 26, 2024
View reviewed changes
WordPress/Classes/ViewRelated/Stats/Subscribers/StatsSubscribersStore.swift
.prefix(quantity)
)
completion(.success(followers))
let combinedSubscribers = Array((wpComFollowers?.topDotComFollowers ?? []) + (emailFollowers?.topEmailFollowers ?? [])
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Based on the answer here pejTkB-1l5-p2#comment-1316 the idea is for all of this aggregation to be replaced by v2/sites/{site_id}/stats/subscribers. I think we can do that both in scope and outside the scope of the project depending on time constraints.

@staskus staskus modified the milestones: 24.8, Pending Apr 26, 2024
guarani
guarani approved these changes Apr 26, 2024
View reviewed changes
Copy link
Contributor

@guarani guarani left a comment

Choose a reason for hiding this comment

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

Works great, thanks @staskus!

@guarani guarani enabled auto-merge April 26, 2024 21:03
@staskus staskus mentioned this pull request Apr 29, 2024
Merged
14 tasks
@guarani guarani mentioned this pull request Apr 29, 2024
Closed
@aditi-bhatia
Copy link

Hi @staskus @guarani, I just wanted to double check something since I'm working on this for Android. Are we planning to implement the % change detail at the bottom of this card during this iteration? I'm not sure if I missed a conversation about this somewhere but I noticed it was in the designs:

Screenshot 2024-04-29 at 10 56 28 PM

@staskus
Copy link
Contributor Author

staskus commented Apr 30, 2024

I just wanted to double check something since I'm working on this for Android. Are we planning to implement the % change detail at the bottom of this card during this iteration? I'm not sure if I missed a conversation about this somewhere but I noticed it was in the designs

Good question, @aditi-bhatia. No, at least not in this iteration. 👍

@staskus staskus modified the milestones: Pending, 24.9 Apr 30, 2024
@guarani guarani merged commit a26b6dd into trunk Apr 30, 2024
25 of 26 checks passed
@guarani guarani deleted the task/23048-stats-total-subscribers-card branch April 30, 2024 07:36
@guarani
Copy link
Contributor

guarani commented Apr 30, 2024

I'm not sure if I missed a conversation about this somewhere but I noticed it was in the designs

I couldn't see the comparison label in the designs mentioned in the issue #23048. I'm just asking to double-check that we're looking at the same designs.

@staskus staskus mentioned this pull request Apr 30, 2024
Merged
14 tasks
@aditi-bhatia
Copy link

aditi-bhatia commented Apr 30, 2024 •
edited

Ah good catch 🤔 I had been referencing this Figma IqhXWz3Iir7RMb5XH5gGfZ-fi-97_5069 from this discussion p1713440407948299-slack-C06BR07TJHK which looks like was a previous iteration. Thanks both!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@guarani guarani guarani approved these changes

Assignees
No one assigned
Labels
Stats [Type] Task
Projects
None yet
Milestone
24.9
Development

Successfully merging this pull request may close these issues.

Total Subscribers Card
4 participants
@staskus @wpmobilebot @aditi-bhatia @guarani