wordpress-mobile / WordPress-Android

WordPress for Android
http://android.wordpress.org
GNU General Public License v2.0
2.92k stars 1.3k forks source link

First version of Calypso Stats #2105

Closed daniloercoli closed 9 years ago

daniloercoli commented 9 years ago

The UI still need to be refined, but major features are all there. We want to start testing new Stats for real asap, and meanwhile refine stats UI with smaller PRs.

Keeping track of opening issues here:

nbradbury commented 9 years ago

@daniloercoli Before reviewing the code I wanted to look at this PR from a usability perspective. As a blogger I found the previous stats to be hard to use and decipher, and on the whole I found these changes to be an improvement.

Most importantly, it's now quicker for me to see how my latest post(s) are doing (I would think that's a big reason people check their stats so often). I also think the ordering of the cards is much better than before (I'm happy I don't have to scroll as much to see referrers).

Last but not least, I like how more data is available now. The inclusion of "Years" as a selection is a nice touch - I don't expect to use that very often, but it's great to have this information when I want it.

With all that in mind, I was confused by a few things in the new stats, and ran into some problems:

daniloercoli commented 9 years ago

@nbradbury - This is only a short answer to all of good points above, that I hope will clear some of your doubts. We started working on this new version of native stats, both on Android and iOS, with the goal of to be in par with web version of Calypso Stats. If you access the new Stats on a mobile browser you will see the same UI, and the same behavior. Would you mind if I post this discussion on our internal p2?

nbradbury commented 9 years ago

Please feel free to share this with everyone else. I actually had the new stats open in the browser while trying this out, so some of what I said applies there as well.

daniloercoli commented 9 years ago

@nbradbury - Thanks again for your feedback. I rewrote below some of the points above, the ones I think are bugs. I can work on these for now, so hold off on review the code.

For the other points, I will share them with my team.

Improvement:

daniloercoli commented 9 years ago

@nbradbury Fixed bugs reported yesterday. UI improvements that regard both web and mobile can be discussed on our P2. Will post a recap there soon. PR is ready for another pass.

nbradbury commented 9 years ago

Those fixes look good, but I think there's a problem with that last one. When viewing stats for a test blog that gets very little traffic (only me) I see the problem shown in this video.

daniloercoli commented 9 years ago

Maybe it's missing the "No activity this period" label on the top of the graph? screen shot 2014-12-16 at 18 33 27

nbradbury commented 9 years ago

I think the problem is two-fold: first, the translucent gray bar above each date makes it look like there are stats for those days, and the translucent orange selection only appears when you tap an empty stat.

nbradbury commented 9 years ago

A bit more feedback on the UI before I jump into the code:

Also, I recommend selecting the "Stats" folder in Android Studio then selecting Analyze > Inspect Code > Current Directory. Although much of what gets reported is harmless, you'll probably find some things worth correcting.

stats-analysis

daniloercoli commented 9 years ago
  • When I tap a bar in the graph, the reflow in the "Posts & Pages" section looks quite bad (demo video). The same thing happens after choosing from the spinner in the action bar.
  • After tapping VIEW ALL on any of the stats sections, the resulting fragment shows a blank row until data is loaded (somewhat similar to the previous issue)

This is because the module is getting in the "loading state", where it's blank with no title in it. See mobile web.

daniloercoli commented 9 years ago
nbradbury commented 9 years ago

If I rotate the device a couple times while years data is loading, I end up seeing a bar chart with only one year visible.

device-2014-12-17-134336

Just in case it's relevant, note that I created my WP blog in 2013, so I only have data for 2013 and 2014.

nbradbury commented 9 years ago

This just started happening: Dec 18 is now showing up in stats, even though it's still Dec 17. Note also that it says I have one like for Dec 18 already.

device-2014-12-17-192104

nbradbury commented 9 years ago

Just noticed that views & visitors (and presumably likes & comments) don't have thousands-separators.

device-2014-12-17-194116

daniloercoli commented 9 years ago

@nbradbury - About the timezone issue you've reported

This just started happening: Dec 18 is now showing up in stats, even though it's still Dec 17. Note also that it says I have one like for Dec 18 already.

Calypso Stats are returned in the blog's timezone setting, not the device timezone. Could you please check the settings in your dashboard?

nbradbury commented 9 years ago

@daniloercoli The timezone for my blog is correct.

screen shot 2014-12-18 at 10 37 32 am

daniloercoli commented 9 years ago

Ok, I've added the timezone issue to the list of things that are still missing.

daniloercoli commented 9 years ago

Timezone issues should be all fixed.

daniloercoli commented 9 years ago

@nbradbury - this PR is ready for another pass.

nbradbury commented 9 years ago

The new UI for Views/Visitors/Comments/Likes works much better and takes care of a lot of confusion I initially had while testing.

I still recommend taking care of many of the issues shown by code analysis, such as "null view root" which is simple to resolve.

null-view-root

I mentioned this earlier and I know it's being discussed, but I wanted to say again that the orange selection highlight in the bar chart is confusing. It only appears on columns that have no data, and it makes it look as though there actually is data. The light gray used to denote empty days is likewise confusing for the same reason.

stats-selection

nbradbury commented 9 years ago

selection

daniloercoli commented 9 years ago

@nbradbury can't fix the "null view root" warning easily. The parent should be null there, since it's assigned later in reloadLinearLayout.

For the issue about empty graph, will probably be changed/improved later, together with the web version.

daniloercoli commented 9 years ago
I recommend testing with a very high-traffic blog such as WordPress.com News. I takes a very long time for data to appear after switching between Weeks/Months/Years

We're aware of this, since we have the same problem on the web. Caching/pre-loading of stats will be introduced later in 2015. It's not a big deal tough, since most users dont have that kind of traffic.

nbradbury commented 9 years ago

can't fix the "null view root" warning easily. The parent should be null there, since it's assigned later in reloadLinearLayout.

Try passing a parent and using false as the last parameter when inflating the view, like this:

inflater.inflate(R.layout.stats_list_cell, parent, false);

This corrects the problem by providing a parent view without actually adding the inflated view to the parent.

daniloercoli commented 9 years ago

Isn't it just a way to remove the warning?

nbradbury commented 9 years ago

That particular warning is actually useful. See this page for an explanation.

daniloercoli commented 9 years ago

Yes, the selection highlight for list items doesn't work fine. Added a bullet for this issue.

nbradbury commented 9 years ago

For the issue about empty graph, will probably be changed/improved later, together with the web version.

I think I'm not explaining myself well. To me the orange selection that only appears on empty bars is a bug that should be fixed in this PR.

If you tap a bar that has data, there's no selection highlight. If you tap a bar that does not have data, there is a selection highlight. That inconsistency is confusing, and it doesn't exist on the web.

On the web the selection highlight always appears regardless of whether there's data. Can the Android app do the same?

nbradbury commented 9 years ago

Related to the code analysis I mentioned, I'm seeing a big increase in build warnings with this branch. The develop branch shows 3 warnings:

develop

Whereas this stats branch shows 34:

stats

daniloercoli commented 9 years ago

Those warnings are about missing strings in 'default translation'. I've added/removed a lot of strings, and now 'default translation' doesn't contain old strings that are still in the localized translation files. This should be fixed once merged in develop by running the update scripts.
Don't know if there is an easy way to fix this without manually removing strings in all localized files.

warning: string 'quick_video' has no default translation. warning: string 'ssl_certificate_do_not_trust' has no default translation. warning: string 'ssl_certificate_trust' has no default translation. warning: string 'stats_comments_summary_footer' has no default translation. warning: string 'stats_comments_summary_header_comments' has no default translation. warning: string 'stats_comments_summary_header_recent' has no default translation. warning: string 'stats_comments_summary_label_most_active_day' has no default translation. warning: string 'stats_comments_summary_label_most_active_time' has no default translation. warning: string 'stats_comments_summary_label_most_commented' has no default translation. warning: string 'stats_comments_summary_label_per_month' has no default translation. warning: string 'stats_comments_summary_label_total' has no default translation. warning: string 'stats_entry_clicks_url' has no default translation. warning: string 'stats_entry_most_commented' has no default translation. warning: string 'stats_entry_search_engine_terms' has no default translation. warning: string 'stats_tooltip_week_of' has no default translation. warning: string 'stats_totals_followers_shares_header_content' has no default translation. warning: string 'stats_totals_followers_shares_header_followers' has no default translation. warning: string 'stats_totals_followers_shares_header_includes_publicize' has no default translation. warning: string 'stats_totals_followers_shares_header_shares' has no default translation. warning: string 'stats_totals_followers_shares_label_blog' has no default translation. warning: string 'stats_totals_followers_shares_label_categories' has no default translation. warning: string 'stats_totals_followers_shares_label_comments' has no default translation. warning: string 'stats_totals_followers_shares_label_posts' has no default translation. warning: string 'stats_totals_followers_shares_label_shares' has no default translation. warning: string 'stats_totals_followers_shares_label_tags' has no default translation. warning: string 'stats_totals_views_per_visitor' has no default translation. warning: string 'stats_video_summary_header' has no default translation. warning: string 'stats_video_summary_label_bandwidth' has no default translation. warning: string 'stats_video_summary_label_impressions' has no default translation. warning: string 'stats_video_summary_label_plays' has no default translation. warning: string 'stats_view_video_plays' has no default translation. warning: string 'stats_view_views_by_country' has no default translation. warning: string 'view_stats_activity' has no default translation.
nbradbury commented 9 years ago

Don't know if there is an easy way to fix this without manually removing strings in all localized files.

Ah, I didn't realize this was the cause of the warnings. I think we can ignore the warnings without resorting to manually removing the localized strings.

nbradbury commented 9 years ago

The changes to the orange highlight look good! I still find it jarring when switching between bars, though - it looks very odd to have the empty rows briefly show up in "Posts & Pages."

screen shot 2014-12-22 at 6 37 20 am

Is there a way to make this transition smoother, so you don't see those unpopulated rows?

daniloercoli commented 9 years ago

Selection highlight issue for list items is now fixed.

nbradbury commented 9 years ago

@daniloercoli The row selection highlight looks great now - very nice.

Looking through your code I noticed several places like this one where an Activity is retained, and that seems to be my fault because an activity needs to be passed here in order to show the reader.

I'd like to correct that as part of this branch, so you can pass a context rather than an activity to that routine (negating the need to hang on to the activity). Is it okay with you if I make the necessary changes and commit them to this branch?

daniloercoli commented 9 years ago
Is it okay with you if I make the necessary changes and commit them to this branch?
sure, feel free to commit to this branch.
daniloercoli commented 9 years ago
Looking through your code I noticed several places like this one where an Activity is retained, and that seems to be my fault because an activity needs to be passed here in order to show the reader.

Now that I'm re-looking at that code, don't know why there isn't a Weak reference there. private final WeakReference<Activity> mActivityRef;

daniloercoli commented 9 years ago

Jarring UI when switching between bars should be gone. i've tried different solutions, but ended up with the simplest one that involves an "expand" animation applied to the list container. Title of the module is also kept on the screen, and not hidden with that grey box.

I've committed 2 fragment animations I was playing with, just for reference and future usage.

nbradbury commented 9 years ago

The animation is an improvement, but it still looks a bit odd. The way the detail quickly vanishes and then expands doesn't look right, and it's especially odd when the data hasn't already been downloaded. See these two videos for examples.

Is it possible to animate out the current data, then animate in the new data?

daniloercoli commented 9 years ago

What if we just load new data and don't use animations https://cloudup.com/c53BuGr9XWw ?

nbradbury commented 9 years ago

Ha! Yes, it looks better that way than it did with the expand animation.

daniloercoli commented 9 years ago

And the following is with slide up / down animation when loading data: https://cloudup.com/c8ox_5TpRaN It needs more tweeks though.

nbradbury commented 9 years ago

I'm not sure about that animation - it doesn't quite fit there. How about sticking with no animation for this PR, then a separate issue to tackle the animation? That way we can wrap this PR up and I can merge it once the Travis build problem is resolved.

daniloercoli commented 9 years ago

I agree with you!

astralbodies commented 9 years ago

FWIW I used this animation in iOS for loading - its not perfect but it seems good. Slide up with existing data, slide down with new data. https://github.com/wordpress-mobile/WordPressCom-Stats-iOS/pull/89

nbradbury commented 9 years ago

:shipit: