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

Stats: Add Subscriber Chart #23074

Merged
merged 9 commits into from
Apr 26, 2024
Merged

Stats: Add Subscriber Chart #23074

merged 9 commits into from
Apr 26, 2024

Conversation

guarani
Copy link
Contributor

@guarani guarani commented Apr 24, 2024

Addresses #23044

I looked to reuse as much of the Views & Visitors card as possible but found that hiding its segmented controls and various labels was difficult. I re-used StatsLineChartView but made a new cell, StatsSubscribersChartCell and a chart configuration object, StatsSubscribersLineChart.

Screenshot

To test

  1. Log into a WP.com account
  2. Enable Stats Traffic and Subscribers tab feature flag
  3. Go to Stats > Subscribers tab on a site and verify the chart shows the same data as on WP.com's stats
  4. Try different scenarios such as no subscribers (new site) and a lot of subscribers
  5. Try pull-to-refresh, connectivity issues, etc.
  6. Compare with design: IqhXWz3Iir7RMb5XH5gGfZ-fi-883%3A4006
  7. Compare with specs: https://github.com/Automattic/wordpress-mobile/issues/47

Known issues

Some updates I'd like to do in follow-up PRs

  • The line chart often looks flat, and small fluctuations in subscribers are not easy to see. The problem is that the y-axis minimum is always set to zero when it should be roughly (or exactly) to the minimum value of the chart.
  • The chart card loading ghost view doesn't resemble the chart
  • The chart tooltip is not set up yet
  • Reduce x-axis gradation
  • Scenarios where there are less then 30 days worth of data returned by /sites/<site id>/stats/subscribers such as a new or recently created site (💥 crashes now on new sites which return one day of data) fixed in 9cc7ba2

Regression Notes

  1. Potential unintended areas of impact

Potentially this P2 could affect the Subscriber email list

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

Manual testing

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

I added a new test to verify the chart card loads correctly

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)

@guarani guarani added this to the 24.8 milestone Apr 24, 2024
@wpmobilebot
Copy link
Contributor

wpmobilebot commented Apr 24, 2024

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 Numberpr23074-55cb3a7
Version24.7
Bundle IDorg.wordpress.alpha
Commit55cb3a7
App Center BuildWPiOS - One-Offs #9699
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 24, 2024

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 Numberpr23074-55cb3a7
Version24.7
Bundle IDcom.jetpack.alpha
Commit55cb3a7
App Center Buildjetpack-installable-builds #8743
Automatticians: You can use our internal self-serve MC tool to give yourself access to App Center if needed.

@guarani guarani marked this pull request as draft April 24, 2024 03:11
@dangermattic
Copy link
Collaborator

dangermattic commented Apr 24, 2024

1 Warning
⚠️ This PR is assigned to the milestone 24.8. This milestone is due in less than 4 days.
Please make sure to get it merged by then or assign it to a milestone with a later deadline.

Generated by 🚫 Danger

Copy link
Contributor

@staskus staskus left a comment

Choose a reason for hiding this comment

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

I re-used StatsLineChartView but made a new cell, StatsSubscribersChartCell, and a chart configuration object, StatsSubscribersLineChart.

I think it makes sense. That's what I would've expected, and it will allow to make subscriber-specific improvements to this chart as well.

The line chart often looks flat, and small fluctuations in subscribers are not easy to see. The problem is that the y-axis minimum is always set to zero when it should be roughly (or exactly) to the minimum value of the chart.

Good observation. It looks like what WP.com is doing, setting the y-axis minimum to the minimum value of the chart. I think it would make sense to do it on the app as well. I opened it up for a discussion (p1713943261341449-slack-C06BR07TJHK)

The chart card loading ghost view doesn't resemble the chart

We need a bunch of new ghost views. I'd say let's leave it outside of the scope of this PR. I added it to #23059 task. When we do all the main functionality we can see how much time remains to perfect loading views.

The chart tooltip is not set up yet

👍 Feel free to split the work into separate PRs if you prefer

Scenarios where there are less then 30 days worth of data returned by /sites//stats/subscribers such as a new or recently created site (💥 crashes now on new sites which return one day of data)

For Traffic charts we generate before/after fake dates for the chart in some cases. Let's say we display a chart for April 1 - April 30, and the site was created April 15. So we create fake April 1 - April 15 dates with value 0, and then the chart grows from there. This is one way to get around some of the problems. If it's a completely new site with 0 subscribers, an empty April 1 - April 30 chart would be displayed.

I also promoted your question in the chart as well p1713943813107969-slack-C06BR07TJHK for a discussion

@irfano
Copy link
Member

irfano commented Apr 25, 2024

FYI: I limited the min and max values of y-axis based on the subscribers count on Android(wordpress-mobile/WordPress-Android#20706).

If the graph doesn't have changes, I set the min value to 0 and the max value to twice the data. This behavior is the same as web stats.

@guarani
Copy link
Contributor Author

guarani commented Apr 26, 2024

FYI: I limited the min and max values of y-axis based on the subscribers count on Android(wordpress-mobile/WordPress-Android#20706).

Thanks @irfano! I'll add this in a follow-up PR along with the chart marker.

@guarani guarani marked this pull request as ready for review April 26, 2024 00:50
@guarani guarani requested a review from staskus April 26, 2024 00:51
Copy link
Contributor

@staskus staskus left a comment

Choose a reason for hiding this comment

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

👍 Thanks! Looks good what we have so far.

Some UI improvements are required but feel free to merge this and do it in a separate PR to be reviewed more easily:

  • Peek interaction
  • Reduce gradation of the chart x-axis
  • Start y-axis from the lowest value
image

@guarani
Copy link
Contributor Author

guarani commented Apr 26, 2024

Thanks!

  • Reduce gradation of the chart x-axis

I added this now to the list of things to do in a follow-up PR 👍

@guarani guarani enabled auto-merge April 26, 2024 14:49
@guarani guarani merged commit 2430ab2 into trunk Apr 26, 2024
25 checks passed
@guarani guarani deleted the task/subscriber-chart branch April 26, 2024 15:03
@guarani guarani mentioned this pull request Apr 30, 2024
14 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.

5 participants