Skip to:
Content

Opened 4 years ago

Last modified 12 months ago

#2587 new defect (bug)

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

Reported by: paulhastings0 Owned by:
Milestone: Future Release Priority: major
Severity: normal Version:
Component: Theme Keywords: needs-patch
Cc:

Description

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

Attachments (7)

2587.001.patch (792 bytes) - added by r-a-y 4 years ago.
2587.002.patch (1.0 KB) - added by r-a-y 4 years ago.
2587.003.patch (1.2 KB) - added by r-a-y 4 years ago.
buy-me-toys.diff (4.5 KB) - added by johnjamesjacoby 4 years ago.
buy-me-toys.2.diff (4.5 KB) - added by johnjamesjacoby 4 years ago.
buy-me-toys.3.diff (5.3 KB) - added by johnjamesjacoby 4 years ago.
activity.diff (5.9 KB) - added by johnjamesjacoby 4 years ago.

Download all attachments as: .zip

Change History (68)

comment:1 r-a-y4 years ago

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

comment:2 johnjamesjacoby4 years ago

  • Priority changed from normal to major
  • Type changed from enhancement to defect

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

comment:3 hnla4 years ago

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

comment:4 r-a-y4 years ago

  • Component changed from Core to Theme
  • Keywords bp-default added; removed

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.

comment:5 johnjamesjacoby4 years ago

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.

comment:6 johnjamesjacoby4 years ago

btw, thanks for taking this on. :D

r-a-y4 years ago

comment:7 r-a-y4 years ago

  • Keywords has-patch added

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!

comment:8 r-a-y4 years ago

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

r-a-y4 years ago

comment:9 r-a-y4 years ago

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

comment:10 paulhastings04 years ago

  • Summary changed from Hide multiple "child-level" comments to [patch] Hide multiple "child-level" comments

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

comment:11 r-a-y4 years ago

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

comment:12 paulhastings04 years ago

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

comment:13 r-a-y4 years ago

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!

comment:14 paulhastings04 years ago

@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-y4 years ago

comment:15 r-a-y4 years ago

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.

comment:16 r-a-y4 years ago

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

comment:17 johnjamesjacoby4 years ago

@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.

comment:18 paulhastings04 years ago

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.

comment:19 follow-up: johnjamesjacoby4 years ago

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

comment:20 in reply to: ↑ 19 paulhastings04 years ago

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?

comment:21 johnjamesjacoby4 years ago

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

comment:22 paulhastings04 years ago

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

comment:23 pisanojm4 years ago

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

johnjamesjacoby4 years ago

comment:24 johnjamesjacoby4 years ago

  • Keywords needs-feedback added
  • Summary changed from [patch] Hide multiple "child-level" comments to [needs-feedback] Hide multiple "child-level" comments

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.

comment:25 paulhastings04 years ago

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.

comment:26 johnjamesjacoby4 years ago

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.

comment:27 johnjamesjacoby4 years ago

Updated patch with improved logic and verbiage.

comment:28 johnjamesjacoby4 years ago

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. :)

comment:29 johnjamesjacoby4 years ago

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.

comment:30 johnjamesjacoby4 years ago

Thinking of putting this on testbp.org. Objections?

comment:31 johnjamesjacoby4 years ago

Added this patch to testbp.org. Please test!

comment:32 paulhastings04 years ago

@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?

johnjamesjacoby4 years ago

comment:33 johnjamesjacoby4 years ago

Try this one

comment:34 paulhastings04 years ago

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

comment:35 johnjamesjacoby4 years ago

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

comment:36 paulhastings04 years ago

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

comment:37 johnjamesjacoby4 years ago

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

comment:38 paulhastings04 years ago

I guess so. :P

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

comment:39 paulhastings04 years ago

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.

:)

comment:40 johnjamesjacoby4 years ago

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

comment:41 paulhastings04 years ago

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.
  1. 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.

comment:42 pisanojm4 years ago

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

comment:43 paulhastings04 years ago

@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.

comment:44 pisanojm4 years ago

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.

comment:45 johnjamesjacoby4 years ago

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.

comment:46 paulhastings04 years ago

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.

comment:48 johnjamesjacoby4 years ago

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.

comment:49 pisanojm4 years ago

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

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

comment:50 hnla4 years ago

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.

comment:51 paulhastings04 years ago

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.

comment:52 pisanojm4 years ago

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?

comment:53 boonebgorges4 years ago

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

comment:54 johnjamesjacoby4 years ago

  • Milestone changed from 1.2.6 to 1.3

Agreed.

comment:55 paulhastings03 years ago

This should be condensed with #2605.

comment:56 pisanojm3 years ago

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.

comment:57 andreaslloyd3 years ago

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...

comment:58 modemlooper3 years ago

Is this getting added to 1.3?

comment:59 DJPaul3 years ago

  • Milestone changed from 1.3 to Future Release

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

comment:60 chochal17 months ago

  • Keywords changed from needs-feedback, activity comments, bp-default, has-patch to needs-feedback activity comments, bp-default, has-patch
  • Severity set to normal

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?

comment:61 DJPaul12 months ago

  • Keywords needs-patch added; needs-feedback activity comments bp-default has-patch removed
Note: See TracTickets for help on using tickets.