-
Notifications
You must be signed in to change notification settings - Fork 1.3k
Fix subscribers chart issues #20776
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
Fix subscribers chart issues #20776
Conversation
|
| App Name | WordPress |
|
| Flavor | Jalapeno | |
| Build Type | Debug | |
| Version | pr20776-e31a090 | |
| Commit | e31a090 | |
| Direct Download | wordpress-prototype-build-pr20776-e31a090.apk |
|
| App Name | Jetpack |
|
| Flavor | Jalapeno | |
| Build Type | Debug | |
| Version | pr20776-e31a090 | |
| Commit | e31a090 | |
| Direct Download | jetpack-prototype-build-pr20776-e31a090.apk |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## release/24.9 #20776 +/- ##
=============================================
Coverage 40.66% 40.66%
=============================================
Files 1490 1490
Lines 68621 68621
Branches 11338 11338
=============================================
Hits 27907 27907
Misses 38195 38195
Partials 2519 2519 ☔ View full report in Codecov by Sentry. |
|
Code LGTM and everything works great. But one thing I noticed. Two cards have the same title, which is less than ideal. I recommend making it clearer. "Subscribers Chart", "Subscribers List", Etc. Since we are here. cc @staskus . This could be the same in iOS. Syncing between the two would be helpful. Or we can decide to ignore it. Up to y'all. |
|
^ I agree with @notandyvee 's feedback, I thought it was strange at first that both cards have the same title - but then noticed that's how it was in the final designs as well. In the earlier proposed designs (IqhXWz3Iir7RMb5XH5gGfZ-fi-108_4611), the name of that card was
Maybe we could change it to that, any thoughts? |
|
Thanks, @aditi-bhatia and @notandyvee, great observation. I think calling it |
|
I also updated the title to the "Subscriber Growth" in this PR. |
aditi-bhatia
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look good to me and work as expected 👍 I left a small clarifying comment on the Flux-C PR related to this to get a better understanding before I approve: wordpress-mobile/WordPress-FluxC-Android#3005 (review)
|
Hi @irfano, I spent some time testing again today and there's a crash that I'm seeing from
I haven't debugged it too much yet (I can help with this tomorrow) but this is the value after setting a breakpoint:
|
|
I couldn't reproduce your crash, but I did fix another issue related to 0 subscribers case. It was crashing when tapping the line on sites with 0 subscribers. This might be related to the crash you're experiencing. I would appreciate it if you could find a way to reproduce it. |
|
Hi @irfano, I can't seem to reproduce the crash I mentioned above which would occur after clicking on
To reproduce:
|
|
That's the crash I mentioned and fixed with my latest commit (9c581d3). Can you ensure that you pulled the latest commit?
|
aditi-bhatia
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used a fresh install last time but maybe something went wrong with the versioning since I'm not able to reproduce the crash today. Tested using a fresh install on two devices, lgtm 👍
|
@oguzkocer, sorry for the delay in making this ready before the code freeze. We should have let you know earlier. Can we include this in |
|
@irfano Sure - but you'll need to wait a couple days for the beta as I just submitted it. |
|
Thank you!
That's fine! This fixes required but minor issues. So, it won't disrupt the testing process until the next beta is released. |
|
antonis
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.












This PR
To Test:
Regression Notes
Potential unintended areas of impact
What I did to test those areas of impact (or what existing automated tests I relied on)
What automated tests I added (or what prevented me from doing so)
PR Submission Checklist:
RELEASE-NOTES.txtif necessary.Testing Checklist (strike-out the not-applying and unnecessary ones):
WordPress.com sites and self-hosted Jetpack sites.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)