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 Post Likes to Post Stats Summary Page #10362

Merged
merged 6 commits into from
Jan 13, 2017

Conversation

youknowriad
Copy link
Contributor

@youknowriad youknowriad commented Jan 2, 2017

Part of #10348

In this PR, I'm adding a new component to the Post Stats Summary Page. This component shows the Gravatars of the post likes and the total.

screen shot 2017-01-02 at 16 37 14

Questions:

  • What if we have a huge number of likes, should we limit the Gravatars to something like the 100 first likes.
  • I used a toggleInfo HoC as a replacement to the old toggle mixin used on other stats components. The mixin used to store its state on local storage using the npm library store. Should I persist this state. I personally don't think it's worth it, and even if it is, I don't think we should add this to the global state tree, maybe we just keep a behaviour similar to the mixin (calling localstorage directly on the component).

Testing instructions

  • try opening the stats summary page for different posts and see how the new component behaves.
    /stats/post/$postId/$siteId

@matticbot
Copy link
Contributor

matticbot commented Jan 2, 2017

@youknowriad youknowriad self-assigned this Jan 2, 2017
@youknowriad youknowriad requested a review from timmyc January 2, 2017 15:42
@youknowriad youknowriad added [Feature] Stats Everything related to our analytics product at /stats/ [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Jan 2, 2017
@youknowriad youknowriad force-pushed the add/stats/post-likes branch from 46bb6e0 to 73fba73 Compare January 6, 2017 10:45
@youknowriad youknowriad added the [Status] Needs Design Review Add this when you'd like to get a review / feedback from the Design team on your PR label Jan 6, 2017
@youknowriad
Copy link
Contributor Author

This is not blocked anymore, so maybe time for a design/code review

@folletto
Copy link
Contributor

folletto commented Jan 6, 2017

Heya! I gave it a go:

  1. One of the posts I tried showed the header but no likes, and not even the message "This post has not likes yet!" — but... it's the homepage of the site (post ID = 0) :D
  2. I'd remove the panel entirely when there are no likes. (either that, or a different text: "There are no likes on this post yet.")
  3. Each avatar has to link somewhere. Either their primary site, or something else more relevant.
  4. Can we move the likes card right below the graph? Thoughts?

What if we have a huge number of likes, should we limit the Gravatars to something like the 100 first likes.

Can we do instead the 100 latest likes which is more useful to see the new people? Also we need to show a placeholder at the end saying "+93" to indicate the list isn't complete (ideally if clicked should still show the full list, if feasible).

@youknowriad
Copy link
Contributor Author

I'd remove the panel entirely when there are no likes. (either that, or a different text: "There are no likes on this post yet.")

@folletto I would prefer fixing the text, because we have to trigger a network request to see if there are likes or not, and during the "requesting" time, we show up a "loading" indicator. If we decide to remove the panel when the loading ends, the user would notice this change and its not the best experience possible (same if we decide to not show the loading indicator and the panel shows up when the request ends)

Can we do instead the 100 latest likes which is more useful to see the new people? Also we need to show a placeholder at the end saying "+93" to indicate the list isn't complete (ideally if clicked should still show the full list, if feasible).

As @timmyc noted on #10361 The endpoint only returns the 90 first likes with avatars. Thus, we won't be able to show the full list.

Time to get my hands dirty and address the feedback tks @folletto

@youknowriad youknowriad force-pushed the add/stats/post-likes branch from 73fba73 to 4a9302a Compare January 9, 2017 10:41
@karmatosed
Copy link
Contributor

I would also second some of what @folletto says. I would like us to consider how we can invisible search (if possible). I'm concerned about the user experience of a loader that doesn't return anything.

If we do only show the 90 first likes I'd like us to find a way of showing more or linking to a place that can with a link. Ideally just having them able to load would be good, but I'm getting the impression that's a limitation.

@timmyc
Copy link
Contributor

timmyc commented Jan 10, 2017

but I'm getting the impression that's a limitation

At this time the 90 limit on gravatar/likees is a limitation. We don't have pagination setup for this in the API, and also I don't think the underlying logic to grab the likes supports pagination either.

export const PostLikes = ( { countLikes, isRequesting, likes, opened, postId, siteId, toggle, translate } ) => {
const infoIcon = opened ? 'info' : 'info-outline';
const classes = {
'is-showing-info': opened,
Copy link
Contributor

Choose a reason for hiding this comment

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

I noticed while the components are loading the module-header-title and infoIcon are both displayed in this new component. If we add in a is-loading class while the query is still loading then this component will match the behavior of the others on the page.

@timmyc
Copy link
Contributor

timmyc commented Jan 10, 2017

One note on the missing loading indicator on the title of the component - also could we add analytics tracking for when a gravatar is clicked on?

Also not sure if we wanted to add this in as @folletto requested:

Also we need to show a placeholder at the end saying "+93"

For grins, this is how the like block looks on a post:

wordpress_com_in_2016__a_year_in review_ _the_wordpress_com_blog

Gravatars in that context are linked to the gravatar site ( i.e. http://en.gravatar.com/timmydcrawford ) - should we do the same here or stick with the user's default site?

@youknowriad
Copy link
Contributor Author

thanks for the review @timmyc

PR updated in d8ba8c9 to address these notes. I added a placeholder with the remaining likes count, but it needs a design review. I hope this addresses the API limitation issue.

screen shot 2017-01-11 at 10 35 50

@karmatosed
Copy link
Contributor

Thanks for the PR update, looking at the screenshot above I can see the + number has some padding issues from size, if you compare the the post links one. I feel they should be the same in format.

I am reluctant to do a full design review sign off until I see this for myself. I have tried to both use mattibot and also /stats/post/$postId/$siteId. On using that though it just refreshed to the blog's insights. I tried on updateomattic. I was wondering if I'm missing a step in trying to recreate to see this PR in action?

@karmatosed
Copy link
Contributor

karmatosed commented Jan 11, 2017

@youknowriad thanks for your help in getting this working. On reviewing a few things:

  • Information text should say 'This panel shows the list of people who like your post'. It currently says likes your post.
  • The +number at end I do think should be made the same size as the post likes number bubble.

2017-01-11 at 12 20

  • With the information block exposed, there seems to be a padding issue to the top:

2017-01-11 at 12 19

  • I wondered about size of the avatar. For example here:

2017-01-11 at 12 21

They are 20 x 20 but in the PR they are 32 x 32. I feel having a uniform size within stats is sensible at least for top level viewing like this is. I'd recommend reducing to 20 x 20. I'm cautious about readability then, but uniformity I think wins out here. The size globally in stats should be something for another PR consideration.

One thing worth noting that I think we all know is a expected and a result of a limitation, I really wanted to click the +number. I found myself doing this several times. If there is no way of loading the rest, this is something we I think have to accept as without the number it looks even odder. That said, it is something I think we should review at some point.

As far as linking to Gravatar this made me wonder if linking to their default blog wouldn't be better. I feel that could be more relevant and more encouraging of engagement than Gravtar.

@youknowriad
Copy link
Contributor Author

Thanks @karmatosed for the review, I updated the component in 98f6ebb. Here is the current design

screen shot 2017-01-11 at 15 09 34

I'd recommend reducing to 20 x 20.

I think you meant 24 x 24 for the gravatar size (at least that's the one used on the component on your screenshot).

As far as linking to Gravatar this made me wonder if linking to their default blog wouldn't be better

Curious to know how do you feel about this @timmyc. Should we revert back to the default blog even if it's not consistent with the post like box.

@timmyc
Copy link
Contributor

timmyc commented Jan 11, 2017

Curious to know how do you feel about this @timmyc. Should we revert back to the default blog even if it's not consistent with the post like box.

I'm fine with either option to be honest. We can always change it later.

@karmatosed
Copy link
Contributor

karmatosed commented Jan 11, 2017

As other links are to primary blog, can we go to that then if no issue?

@youknowriad sorry yes, read wrong number. That looks much better. I'll give it a final spin once we get that link change maybe in.

@timmyc
Copy link
Contributor

timmyc commented Jan 11, 2017

As other links are to primary blog, can we go to that then if no issue?

👍 - I think the only thing we need to do is check for existence of primary blog, then maybe fallback to Gravatar. Thinking of my dear mom who randomly likes my blog posts, but does not have a blog 😆

@karmatosed
Copy link
Contributor

@timmyc excellent point about defaults. That makes sense and lets make sure your mum still has a link :)

@timmyc
Copy link
Contributor

timmyc commented Jan 12, 2017

Tested this out again - I think once the remaining URL issue is taken care of, this is good to 🚢 - a rebase might be good too 👍

@youknowriad
Copy link
Contributor Author

Link updated in 78c7dc4 :)

@timmyc
Copy link
Contributor

timmyc commented Jan 12, 2017

:shipit:

@timmyc timmyc added [Status] Ready to Merge and removed [Status] Needs Design Review Add this when you'd like to get a review / feedback from the Design team on your PR [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Jan 12, 2017
@karmatosed
Copy link
Contributor

This looks great. I have just one bit of feedback and it's about padding at the bottom. I almost feel compared to other sections we need to have more?

2017-01-12 at 16 21

@youknowriad youknowriad merged commit 9865d1b into master Jan 13, 2017
@youknowriad youknowriad deleted the add/stats/post-likes branch January 13, 2017 09:02
@karmatosed
Copy link
Contributor

karmatosed commented Jan 13, 2017

Thanks @youknowriad and @timmyc - so great to see this completed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Stats Everything related to our analytics product at /stats/
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants