[needs-feedback] Hide multiple "child-level" comments

[paulhastings0]

If there's a quick fix for this (I imagine there is) then it'd be nice to include in the 1.2.6 release.

Basically the activity comments hide all but the last 5 1st-level replies, but it doesn't factor in the number of 2nd-level replies.

In the below video example there's a discussion with 156 replies, but only 12 or so of those replies are hidden. Which can get really annoying for readers who aren't interested in that discussion and simple want to scroll past it.

​http://screencast.com/t/NTQ0NTU3M

[r-a-y]

I'd like to see activity comments use the Walker class that WP uses for comments.

[johnjamesjacoby]

Valid point. Switched from enhancement. Still love the screencasts!

[hnla]

Just have to say: loved the presentation :)

And as for the issue 'Yikes!' definitely a defect

really
difficult
to
scan
comments
when
they
get
to
this
point

[r-a-y]

Okay, so I actually watched the screencast this time ;)

It's a javascript issue in the bp-default theme. I'll take a crack at this one.

---

JJJ, just a dev question, but right now all activity comments are rendered in the $activities_template global variable. Does that impact performance in cases like Paul has?

If he loads the activity stream front page, that activity update + comments (all 155 of them) will be rendered plus all the other activity updates + comments.

[johnjamesjacoby]

There's probably arguments for both sides, but yeah; no sense in loading and processing a bunch of things that aren't even visible if we can help it.

Either we preload them and hide them so they are on the ready, or we only load what we need and ajax load the complete thread on request. I would be happy with either situation over what is happening now, but I think the later makes the most sense as a best case scenario.

For 1.2.6 I would accept a patch to toggle the child comments, knowing for 1.3 we should revisit the way this actually functions.

[johnjamesjacoby]

btw, thanks for taking this on. :D

[r-a-y]

Quick patch... haven't tested with 155 nested activity comments mind you ;)

If you have nested comments, it will simply hide the parent activity comment.
Therefore, you would not see the latest five parent comments, but four.

If anyone has a more, elegant solution, please modify the patch to your liking!

[r-a-y]

Crap... read that an accepted patch would require toggling the child comments.
Back to the drawing board!

[r-a-y]

Updated patch adds toggling for child comments only for the last five parent activity comments.

[paulhastings0]

I'll try testing this out tonight as soon as my users go to sleep. Thanks @r_a_y!

[r-a-y]

If you can't wait for your users to go to sleep, here's a screenie :)
​http://img718.imageshack.us/img718/2392/childcommentstoggle.png

[paulhastings0]

Ah, cool. If I may be picky, shouldn't it run some type of test if there are 2 or more child comments? Instead of saying "see all 1 comments" it would be more grammatical to say "see this child comment" or something like that... just being picky. :)

Besides that it looks decent enough for 1.2.6

[r-a-y]

It uses BP's JS terms and there isn't a term for singular conditions.
The "See all 1 comments" thing applies if there is only 1 child comment.

Good to be picky though!

[paulhastings0]

@r-a-y I finally tested it after everyone went to bed and at first blush it seems to work fine. However I did notice a couple of glitches:

1. When viewing the activity stream it above the "Load More" it works great, but once I click "Load More" and look at what's loaded it reverts back to the old way of showing all the replies. In fact it's a little worse because this time it doesn't even attempt to hide any of the 1st-level child comments (even if there are 9).

2. The 2nd thought is more of an enhancement than a bug I suppose. After clicking "Show all zz comments" there's no way to hide the comments again. So if I accidentally click "Show all 39 comments" then there's no way for me to hide those 39 comments which forces me to scroll down past 39 comments that I don't care about.

Anyway, I'm off to bed. I think I may have an infection so I need some sleep. :D

[r-a-y]

Thanks for testing, Paul.

New patch fixes issue #1 and adds child comments nesting for all root activity comments as well.

As for issue #2, can you create a new ticket for collapsing comments? I'll look at that when I have some more time.

[r-a-y]

I meant to write child comments toggling, not nesting!
Okay, sleepy time!

[johnjamesjacoby]

@paulhastings0 - Since you seem to have a great setup to test this on, can you explain what your ideal setup would be?

You're right to say the show/hide toggle would probably be an enhancement for 1.3. Let's get the showing done first.

[paulhastings0]

Loading unseen comments via AJAX would be sweet because it would speed up the initial page-load speed.

@r-a-y Good idea. Here's the 1.3 ticket: #2605

@johnjamesjacoby The ideal situation is more theoretical I suppose. I run ​JibeNow a social network for Christian homeschoolers and we use the ​BuddyPress Activity Stream Bump to Top plugin from @nuprn1. Every time someone replies to an update the update is bumped back to the top of the activity stream.

As you've seen this can create extremely long conversations sometimes. A good portion of the users won't have any interest in the conversation though and just want to scroll past it as quickly as possible. Some of them have suggested that "all" or "all but the last 2"activity replies be hidden by default and only revealed when a "show replies" tab is clicked. That would make skimming the site much faster.

[johnjamesjacoby]

Have a slick fix in the works for this for 1.2.6.

[paulhastings0]

Replying to johnjamesjacoby:

Have a slick fix in the works for this for 1.2.6.

Cool! It'll apply not only to the main activity stream but also to when viewing individual user's activity streams, right?

[johnjamesjacoby]

Right. It's stream agnostic so anywhere there is a stream, it will all work the same.

[paulhastings0]

Cool, let me know when it's time to test it out. :)

[pisanojm]

Ok... so is there a new patch for this? Did I miss it? I'd be happy to test it as well.

[johnjamesjacoby]

Attached patch is an attempted fix. There are unused 'threshold' variables that, if we can agree this is a better direction, would be used to let site admins set the activity thresholds for when to hide root level replies, and child replies.

Feedback needed.

[paulhastings0]

I won't ask about the title of the .diff. :P

Just to clarify, we're already running the last patch submitted by @r-a-y. I'm using Tortoise SVN on my "home computer" to keep track of revisions and then uploading the affected files to our serveru via FTP.

Do I need to revert my "home computer" files first, before I run run your .diff on the "home computer" files?

Sorry about the ignorance here, I'm just getting used to this SVN thing.

[johnjamesjacoby]

Right. My patch is off of a clean working branch without any other modifications on it. You would want to be using a check-out of the latest code in the 1.2 branch of code to guarantee it working correctly.

[johnjamesjacoby]

Updated patch with improved logic and verbiage.

[johnjamesjacoby]

Most of my diff titles or descriptions are obscure pop-culture references, mostly about movies I'm watching in the background of my day at the time. :)

[johnjamesjacoby]

Further refined version with added styling and smoother scrolling support. Fixed support for small and large thresholds, although those thresholds will not be admin assignable in 1.2.6.

Note: Template pack would need to be updated to support these changes.

[johnjamesjacoby]

Thinking of putting this on testbp.org. Objections?

[johnjamesjacoby]

Added this patch to testbp.org. Please test!

[paulhastings0]

@jjj

I keep trying to apply the patch you uploaded but my SVN client won't accept it. I'm using Tortoise SVN and after selecting "Apply Patch" and navigating to your file the SVN client will open up but won't show me the patch tool.

In fact it won't even try to confirm with me that I'm in the correct folder. I tested some other patches and diffs and they seem to be working just fine. The problem just appears to be occurring with your 3 patch files on this page. Is it possible there's a typo somewhere?

[johnjamesjacoby]

Try this one

[paulhastings0]

Nope, still nothing at all. Shall I make a quick screencast?

[johnjamesjacoby]

Hm sure? Otherwise I have it up on testbp.org if you want to see the idea.

[paulhastings0]

Ok, I'll have something up in 5 minutes. Dude, you're up late. It's like, 4am where you are?

[johnjamesjacoby]

Such is the glamorous life of a fulltime developer. :)

[paulhastings0]

I guess so. :P

Here's my experience summed up in 2 minutes: ​http://screencast.com/t/NTY2M2VmY

[paulhastings0]

Well... um... I think I may have crashed the testbp.org homepage. I was testing out the activity replies (which looks very cool btw), but after my 12th 1st-level activity reply the home-page went blank.

Other pages on the site are showing up ok, but any pages that showed my testing (like the BuddyPress testers group activity stream) are just showing up as blank white.

:)

[johnjamesjacoby]

Interesting. I deleted those activity items for now to bring the site back up. Must have some conflicting code somewhere.

[paulhastings0]

Well, besides that bug, the rest of the functionality looks cool. From a user point of view I might suggest a couple of changes.

1. After clicking "Show X replies" it'd be good to generate a "Hide X Replies" button that does the opposite.

2. The activity stream is still showing all 1st-level replies. If 20 or so users leave 1st-level replies then it kind of defeats the purpose of hiding lower-level replies. I'd probably try to limit 1st-level replies to the last 5 or so, and after that then show a "Show X Replies" button.

[pisanojm]

I just made a 35 reply on testbp.org... See this direct here:
​http://testbp.org/activity/p/88392/

LOVE the show reply expansion...

I believe that you should show the 1st reply or couple, else make it selectable in the settings somewhere.

Also, agree with Paul above should have the ability at the bottom of the replies to "hide replies" again, posted at the bottom of the last reply.

I also didn't know, since you were dealing with this exact area of code if you didn't want to tackle the attached, somewhat related, ticket as well at the same time (you can see where I had an issue posting the last time with the above link as well)....

​http://trac.buddypress.org/ticket/2230

[paulhastings0]

@jjj So is there any possibility we can get this out of the door?

I know a lot of people (including myself) have been waiting on Jeff's Privacy plugin for the better part of a year now, and Jeff assures us that he just needs 1.2.6 to be released with his patches (which I believe were all accepted).

I'm ready to do more testing as necessary.

[pisanojm]

I just re-tested this... you can see it here...

​http://testbp.org/members/pisanojm/activity/88743

Looks like it works fine...
Thoughts:
Would like to see the ability to "close" as stated above.
Would like to see the 1st tear of replies by nested as well...

None of these are deal-breakers.... maybe future enchancements for 1.3 based on what you release for 1.2.6...

I'm getting "antsy" for the release as well... but I do appreciate you all throughly taking the time to deal with the issues and properly test.

[johnjamesjacoby]

I'm antsy too. :)

What do you mean by "Would like to see the 1st tear of replies by nested as well."

Just needs a few tweaks and it's probably ready to ship.

[paulhastings0]

By "1st tier" he means "1st-level". With the current patch if I make 20 1st-level replies all 20 of them will show up. None of them are being hidden.

In my community it's pretty typical to see anywhere form 20-30 any-level replies. Although occasionally we'll have a monster conversation with 100+ any-level replies.

[paulhastings0]

"from"

[johnjamesjacoby]

Ah that makes sense. Yeah I'll bring that back and work on the Open/Close also. Won't get a chance until next week but will get it in.

[pisanojm]

Right, "1st tier"... long day... :)

Looking forward to seeing the next evolution of these changes...

[hnla]

Guys this one ticket has been running for 6 weeks and appears to be holding up the release of 1.2.6 albeit for RC testing. 1.2.6 has some much needed patches.

This ticket is an 'Enhancement' type it's not a 'Defect'. That all comments visually display is not a defect you are meant to be able to see comments! that horrendous numbers of comments at various levels upset the visual layout of a page is more a case of controlling how comments are made. providing a means to show /hide levels of comments is a good idea but is an 'Enhancement'

The priority status of this ticket is also open to debate, to mark it as 'Major' is questionable and it ought to be set at 'Normal'

If there is a 'Defect' in all this it rests with BP and it's handling of nested comments? is there not a reason that one can set a particular number of permitted levels with WP comments?

Comments of comments of Comments of Comments are not conversations and actually rapidly become impossible to follow.

I'm not in any way attempting to denigrate the work that has gone into this and agree with the principle that has been explored in show/hide, but I do feel that a little perspective needs to be brought to the proceedings.

[paulhastings0]

You may have a point there and I'm just as anxious to get 1.2.6 out the door. Obviously we all agree that something needs to be done about the way that activity replies are shown.

If it can quickly resolved, or if it's not the only ticket holding off the 1.2.6 release then I say let's resolve it. But if this ticket still can't be resolved and if it's the only thing holding off the 1.2.6 release then by all means, let's punt it.

[pisanojm]

Hey all BP coding gurus and controllers of the 1.2.6 release destiny and timeframe.... any more thoughts on this and moving it "out the door".

Again, maybe consider releasing "as-is-now" and table suggested development as an "enhancement" to version 1.3?

[boonebgorges]

I agree with hnla that this seems very much like an enhancement and shouldn't hold up 1.2.6.

[johnjamesjacoby]

Agreed.

[paulhastings0]

This should be condensed with #2605.

[pisanojm]

Hello all,

Just checking back in to see the progress of this since we left it off a couple of months ago... Please ping me on buddypress.org when you get a new patch rolling... I will be happy to test.

[andreaslloyd]

Hi all,
I'd just echo pisanojm, and say that I'd love to see this issue fixed. Let me know when you have a new patch that needs testing...

[modemlooper]

Is this getting added to 1.3?

[DJPaul]

I can't figure out what patch is still relevant or remember what exactly the issue with the most recent patches were, so I'm bumping this to a future release unless JJJ can spend some time on it

[chochal]

Any way to make this work for buddypress 1.5.x or 1.6.x? Especially the activity.diff patch? The global.js has changed a bit in 1.5.x onwards, particularly the function bp_dtheme_hide_comments so I'm not quite sure how to implement the aforementioned patch?

[DJPaul]

This old ticket is in what we'd now call the template pack (it was in bp-default at time of ticket creation). Since this hasn't seen any traction in years, I'm going to say we don't want to make such a change to BP-Legacy at this point in time.

Maybe it's something that can be considered for a future template pack if those contributors fancy this kind of feature.