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

Reader: Convert post comments to use redux state #3528

Closed

Conversation

samuelclemens
Copy link
Contributor

The required modifications for reader's post comments react components to work with redux state.

WIP, Dependent on #3143 (it included here meanwhile, for the whole thing to work)

Still needs:

  • Design for pingbacks/trackbacks
  • Design for empty state (no comments), looks kinda empty.
  • Decide on comment count to collapse ( currently 1 )
  • Convert other references to CommentStore CommentLikeStore to redux state
  • Fix bugs

Will close #967, #2839

screenshot from 2016-02-24 06-04-22

@blowery blowery changed the title Convert reader post comments to use redux state Reader: Convert post comments to use redux state Feb 24, 2016
@lancewillett lancewillett added OSS Citizen [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. [Feature] Reader The reader site on Calypso. labels Feb 25, 2016
@blowery
Copy link
Contributor

blowery commented Feb 26, 2016

@jancavan Could you please work up designs for pingbacks and the empty state?

@jancavan
Copy link
Contributor

I'm having trouble checking this branch out, but here's some feedback while I try to resolve this:

  1. on the image above, the Reply button under each comment has disappeared?
  2. 2nd/3rd-level comments should be collapsible after they'd been expanded (this may already be the case, but this example made me think that once they'd been expanded, they're no longer collapsible until after a browser refresh)
  3. "Show/Hide" comments instead of "View/Hide"
  4. if there's only 1 reply, it should just say "1 reply" instead of "1 replies"
  5. to make tap areas much larger, the entire blue bar should be tappable on mobile

Posting an idea I experimented with, others may want to chime in:

To make nested comments easier to scan, thought of adding a left border before the indented comments. Although this makes nested comments much easier to scan, it buries the "Show/Hide" links. Using the horizontal bar from the original mockup alongside the left border only adds clutter.
comment-border-left

So I say let's just stick to the original design:
expand-collapse-comments

Still working on pingbacks. For empty state, this may be a good opportunity to add something that's more delightful than a plain form field, but for now can we at least have the comment field say:
screenshot 2016-02-26 16 07 17

@blowery
Copy link
Contributor

blowery commented Feb 27, 2016

For empty state, this may be a good opportunity to add something that's more delightful than a plain form field, but for now can we at least have the comment field say:

On the actual blogs, I think that message is configurable. Maybe we try to grab it and use it here?

For example:
screenshot

from http://blog.lostartpress.com/2016/02/26/prelude-to-perfection/#reply-title

@samuelclemens
Copy link
Contributor Author

@jancavan First of all, thanks for the detailed review.

  1. The missing reply button is my bad, it's fixed on the latest version
    of this branch.
  2. That is correct, once the user showed the replies, there was no way
    of him collapsing the comments.
    In the latest version of that branch I've adjusted it according to your animated gif, leaving the hide button clutters the UI a bit though.
  3. I've implented this in the latest version of this branch.
  4. I've implented this in the latest version of this branch.
  5. It is already so, the blue bar is a <button />

I kinda like the left border, maybe it should be mouse hover/tap kind of thing?

Regarding the empty state, except the 'no comments' state I also mean
this scenario:
screenshot from 2016-02-27 23-12-00

When there are no more comments to load ( no 'view earlier' link ) it kind of looks empty.

@blowery
Copy link
Contributor

blowery commented Feb 29, 2016

When there are no more comments to load ( no 'view earlier' link ) it kind of looks empty.

I think we just drop the bar when there's no need for it.

@samuelclemens
Copy link
Contributor Author

What about the comments count 4 of 4 ? drop it as well when there's no more earlier comments? It'll be a bit weird having it at the beginning and then removing it at the end.

@jancavan
Copy link
Contributor

When there are no more comments to load ( no 'view earlier' link ) it kind of looks empty.

It's fine if there's no "View earlier" link. We shouldn't add something just because it's empty :)

How many comments are displayed until this link pops up?

@samuelclemens
Copy link
Contributor Author

@jancavan The link displayed as long as there are more comments to load,
on first load there's 3 "root" comments, each click on the view earlier will bring 5 more "root" comments.
(It's configurable.)

@jancavan
Copy link
Contributor

I think 5 "root" comments is a good number to display by default. Then "View earlier comments" loads up the next 10 or so.

Couple of issues I have:

  1. because we are showing the most recent comments by default, this doesn't provide enough context of the discussion and...
  2. this inadvertently hides comments that have lots of likes and comments which doesn't really contribute to prolonging a conversation - if anything, it actually puts an end to it.
  3. I have to scroll down to get to the comment form.

These are issues we can tackle later, but just wanted to note it down here. Displaying it in chronological order is great, but displaying most-liked and most-commented-on comments may also help spark or further a discussion.

@jancavan
Copy link
Contributor

Here's the design for pingbacks:
pingbacks001

I'm using gridicons-link on here.

Can we change the format to [ blog title ]: [ post title ] instead of [ post title ] | [ blog title ] or [ post title ] « [ blog title ]? I think those are messy to look at.

@jancavan
Copy link
Contributor

jancavan commented Mar 1, 2016

What about something like this for empty state comments?

Edit: I'm still leaning towards having nothing but the comment field (but wanted to try it out anyway to see how it'd look) as we should eliminate distractions from this page and just let users focus on reading the post. Changing the value of the comment field to "Be the first to leave a comment" if there aren't any comments yet is I think a good addition though.

(A)
drake001

(B)
drake002

@jancavan jancavan 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 Mar 1, 2016
@samuelclemens
Copy link
Contributor Author

I've adjusted the initial "page" size to 5, and then load "pages" of 10 comments.
I've implemented the trackback design.
I've addressed @jancavan's comments regarding style.scss

Regarding the title, in my opinion the one who decides the format is the pingbacking blog, not us, since I've seen it in various formats (blog name first, then last, with | separator and dash separator, etc.) but i'm no really sure about that.

I really like drake with the pencil =) But maybe make him a bit smaller?
Having just the form and dropping the bar when there are 0 of 0 comments is fine as well I guess.

@blowery Any idea if the comments message available anywhere on wpcom api?

@blowery
Copy link
Contributor

blowery commented Mar 2, 2016

Any idea if the comments message available anywhere on wpcom api?

@samuelclemens Don't worry about it for now. The more I think about it, the less I like it, as the user isn't going to attribute that bit of content back to the source site, but to the Reader itself. I'd rather have it consistent and clean, rather than give the site admin a vector for something confusing.

@jancavan
Copy link
Contributor

jancavan commented Mar 2, 2016

@samuelclemens Okay, let's just leave them as they are for now if this isn't doable.

The reply bars could use some margin and padding needs to be reduced.

Current:
screenshot 2016-03-02 13 04 41

Updated:
screenshot 2016-03-02 13 05 01

Reduced the top padding to 5px and added a 15px top margin.

@blowery
Copy link
Contributor

blowery commented Mar 2, 2016

@samuelclemens Can you please rebase this now that the redux pieces have landed? Thanks!

@jancavan
Copy link
Contributor

jancavan commented Mar 4, 2016

Let's forego adding anything to the comment field for now and just revisit it later. This is great, but looking at it more, not a big fan of adding pingbacks really as they look messy. Maybe there's a way to make them less highlighted or something, I'll think about it.

@samuelclemens samuelclemens force-pushed the add/redux-comments-ui branch 3 times, most recently from f2529e7 to fe50312 Compare March 4, 2016 09:56
@samuelclemens
Copy link
Contributor Author

The thing with pingbacks, is that they are included in the total comments count, there is currently no easy way of splitting that count.
It'll be weird counting them and not showing them, we also discussed it here.

@samuelclemens
Copy link
Contributor Author

I've looked a bit into how pingbacks work here. Author name (what we display as the link text to the pingbacked blog) is the title extracted directly from the remote blog, it may (or may not) contain anything really.
The comment content is the text with which remote blog linked to us.

@jancavan
Copy link
Contributor

jancavan commented Mar 4, 2016

Yup, that's why I didn't suggest on removing them, I understand they had to be included to make comment count possible. I was just thinking that maybe they can be less prominent, especially when there's a bunch of them in a row. I'll have to think about it.

@jancavan
Copy link
Contributor

jancavan commented Mar 9, 2016

Reiterating for completeness:

  1. Let's leave the comment bar that don't have "View earlier comments" as it is. There's no need to add something there. But can we change the comment field value to "Be the first to leave a comment" when there aren't any comments at all yet?
  2. Current empty state is fine for now, let's not add Drake. We can revisit later.

A few more comments, sorry didn't notice these earlier:

  1. 3rd-level of comments should be indented and aligned with the reply bar above it:
    screenshot 2016-03-09 10 11 08
    In general, the reply bars are confusing me now looking at it more. I'm going to see if there's a different way we can do this to make it less confusing.
  2. I don't think there's a need for the "Cancel reply" link. If users decide not to post their reply, then they wouldn't hit "Send". It just clutters up the UI with something of little value.
  3. There seems to be a bug when I enter a comment. It shows up as a new reply, but there's nothing in it. The comment I left also stays in the field:
    screenshot 2016-03-09 10 34 48
  4. When I hit reply on a comment, the comment field shows up. Without posting a reply, I hit reply again on another comment, but the field only shows up if I click the reply link twice.

@samuelclemens
Copy link
Contributor Author

Hi,
Regarding the comments above:

  1. It seems aligned over here, what browser are you using?
    screenshot from 2016-03-13 01-23-45
  2. I think the link is there for the user to return to the mode where they comment directly on the post, and not replying some comment, without that there is no way to go back. So if it'll be removed we'll need to replace it with something else.
  3. There were a bug with clearing the comment input, it should be fixed now, but I'm not sure what you meant in the second part. You started typing a comment and then it immediately appeared as a replay? Or you submitted it ( clicked send ) and it showed up as an empty comment? Something else?
  4. The same issue appears now on production ( to reproduce aim for the bottom end of the "replay" button of a lower than current active replay), imo It's related to the min-height of the focused input, my theory is:
  • User types a replay
  • User clicks on a "replay" button
  • blur event is emitted, input resizes back to original size
  • all the elements below input move up, which means "replay" button moves up
  • the click event is hitting nothing, the button is no longer there.

I've added a somewhat hackish fix to it: added delay in the component itself on the blur event, so the min-height will apply some milliseconds later.

@jancavan
Copy link
Contributor

It seems aligned over here, what browser are you using?

My bad, I got confused with the comments after they exceeded 3-levels.

That said, we're already not indenting after 3-levels now, but because the count still exists at that point, omitting indentation results to lost hierarchy. It's also not a great experience on mobile because they get really narrow.

Like in this example, it's difficult to distinguish if fourth level reply is supposed to be under third level because it also looks like it's a reply for second level.

screen shot 2016-03-15 at 10 49 01 am

I think we should set a max threshold for counts and indenting. We could keep indenting until 2-levels, but stop at anything beyond that and additional replies will be added to the 2nd-level count. /cc @blowery

Example:
redux001

I think the link is there for the user to return to the mode where they comment directly on the post, and not replying some comment, without that there is no way to go back.

Yeah, I was thinking to just keep the main comment form on there at all times. I thought of moving "Cancel reply" to the right which makes it slightly better visually. However, the problem with this is type of UI regardless of where we put cancel is if someone accidentally clicks/taps it, then their comment is gone. It's especially not optimal for mobile.
reply

So I think the main comment form should always be there. I think adding the left border back in helps tell the two forms apart in instances where they're next to each other.

Example:
https://cloudup.com/cwV8cth1I5E

Ideally, the main comment form should be at the very top of the comment listing, but since we order them chronologically, it'll feel disjointed if they type a comment up top and they appear at the bottom after submitting. I also thought of just anchoring them to their comment after submitting, but doesn't make for a great experience either.

@blowery
Copy link
Contributor

blowery commented Mar 15, 2016

I think we should set a max threshold for counts and indenting. We could keep indenting until 2-levels, but stop at anything beyond that and additional replies will be added to the 2nd-level count.

That makes sense but it also makes my head hurt. It's breaking expectations and possibly conversational flow in the original site. Though perhaps it's fine in practice...

Just imagine some our p2 threads with only two levels of conversation. :)

@jancavan
Copy link
Contributor

Right, make sense, but the nesting isn't scalable and only solves the problem up to a certain point so we still need to set a limit somehow. I guess indenting up to 3-levels (instead of 2) isn't too bad. And normally, the replies that are several levels deeper are no longer directly relevant to the "root" comment so should be fine.

@blowery
Copy link
Contributor

blowery commented Apr 15, 2016

I guess indenting up to 3-levels (instead of 2) isn't too bad. And normally, the replies that are several levels deeper are no longer directly relevant to the "root" comment so should be fine.

I noticed that the native apps already do this condensing of levels and limit it to 3 total (1 main, two nested).

@bluefuton
Copy link
Contributor

I'll be continuing this work in a new PR - #4817.

@bluefuton bluefuton closed this Apr 18, 2016
@scruffian scruffian removed the [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. label Apr 18, 2016
@bluefuton bluefuton removed the [Status] Needs Design Review Add this when you'd like to get a review / feedback from the Design team on your PR label Apr 18, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Reader The reader site on Calypso. OSS Citizen
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Reader: hide replies initially for threads with lots of comments
7 participants